LFG: make the dungeon finder work end to end - #82
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afb8fb9b33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // So: mark it failed, tell everyone still listed so their windows close, run the | ||
| // per-player teardown, then erase the proposal unconditionally. | ||
| if (plrAnswer == LFG_ANSWER_DENY) |
There was a problem hiding this comment.
Reject proposal responses from non-members
When a modified client sends CMSG_LFG_PROPOSAL_RESPONSE with an active proposal id that does not belong to them, this path still inserts their GUID into proposal->answers and treats accepted=false as a real decline, so any logged-in player who guesses or observes the global proposal id can cancel another group's proposal and clear the queued members. Check that plrGuid is already present in the proposal's participant/answer maps before honoring either an accept or a decline.
Useful? React with 👍 / 👎.
| // still LFG_STATE_QUEUED next tick, gets matched again, and fires a fresh proposal | ||
| // -- and a new SMSG_LFG_PROPOSAL_UPDATE -- every single tick, forever. | ||
| m_queueSet.erase(guid); | ||
| m_playerData.erase(guid); |
There was a problem hiding this comment.
When any recipient never sends a proposal response (for example they ignore the popup, disconnect, or the client-side timer expires), there is no LFG_TIME_PROPOSAL reaper in LFGMgr::Update, and this line has already removed the only queue data that could put the players back. The remaining members stay in LFG_STATE_PROPOSAL indefinitely, and JoinLFG now refuses that state, so they can be stuck out of dungeon finder until a relog or manual leave path clears them.
Useful? React with 👍 / 👎.
| // Add group to our group set and group map, then teleport to the dungeon | ||
| ObjectGuid groupGuid = pGroup->GetObjectGuid(); | ||
| LFGGroupStatus groupStatus(LFG_STATE_IN_DUNGEON, dungeon->ID, proposal->currentRoles, pGroup->GetLeaderGuid()); | ||
| pGroup->AddMember(it->first, pMember->GetName()); |
There was a problem hiding this comment.
Convert raid-sized proposals before adding members
When the matched dungeon's DBC quota is larger than five (the new role resolver explicitly allows LFR/flexible raid sizes), the newly created group is still a normal party, so Group::IsFull() makes AddMember fail after MAX_GROUP_SIZE; this return value is ignored here. The extra players have already accepted and are sent the success/leave updates, but they are not added to the group or teleported, so raid-finder-sized proposals complete as a partial five-player party.
Useful? React with 👍 / 👎.
| @@ -307,28 +320,27 @@ bool LFGMgr::IsProposalSameGroup(LFGProposal const& proposal) | |||
| continue; | |||
There was a problem hiding this comment.
Abort proposals with offline members
When a queued player logs out before a completed match is proposed, this skip lets proposal construction continue without that member in groups or answers, while currentRoles still counts them as part of the completed composition. The remaining online players can all accept, allOkay sees no pending answer for the offline member, and CreateDungeonGroup builds/teleports a short group while recording status for someone who was never added; remove or fail the stale queue member instead of silently ignoring them here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Server/src/game/WorldHandlers/LFGHandler.cpp
Line 145 in e4cd350
Passing isGroup=true for every grouped caller sends non-leaders into LFGMgr::LeaveLFG's group branch, and that branch does not check pGroup->IsLeader(...) before iterating every member and removing the group queue entry. In a queued, role-check, or proposal state, any party member who sends CMSG_LFG_LEAVE can therefore cancel dungeon finder for the whole party; either keep the leader check here or enforce the permission before the group-wide removal.
Because roleCheck is now a reference to the stored map entry, this subscript persists insertions for any current group member who was not part of the original role check. If someone is invited after the role check starts, they can send CMSG_LFG_SET_ROLES and either abort the pending check or get queued without the join-result and dungeon-lock validation that ran for the original members; verify plrGuid already exists in currentRoles before accepting the role update.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false; | ||
| } | ||
|
|
||
| LfgDungeonsEntry const* dungeon = sLfgDungeonsStore.LookupEntry(*dungeonList.begin()); |
There was a problem hiding this comment.
Evaluate each selected dungeon's role quota
When a player queues for multiple specific dungeons whose DBC role counts differ, this helper validates the entire selection against only the lowest-sorted dungeon id. The code now explicitly supports TypeID 1 rows that include 0/0/3 scenarios, 0/0/1 solo content, and 1/1/3 dungeons, so a valid role set for one selected dungeon can be rejected just because another selected dungeon happens to sort first; the matchmaker should test/filter the compatible dungeon set instead of letting begin() decide the whole queue entry.
Useful? React with 👍 / 👎.
| if (!pMember || pGroup->IsMember(it->first)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Abort when an accepted proposal member disconnects
If a participant accepts the proposal and then logs out before the final response arrives, allOkay still succeeds because their answer is already AGREE, but this path silently skips the offline member and continues creating/teleporting the dungeon group. That is a separate post-proposal disconnect case from the pre-proposal online check: it still produces a short group while groupStatus records roles for someone who was never added, so the proposal should be cancelled or requeued instead of continuing past a missing accepted member.
Useful? React with 👍 / 👎.
| // -- and JoinLFG refuses that state, so they could not re-queue until relog. | ||
| for (std::vector<uint32>::const_iterator it = expired.begin(); it != expired.end(); ++it) | ||
| { | ||
| CancelProposal(*it, std::set<ObjectGuid>()); |
There was a problem hiding this comment.
Remove timed-out players before requeueing survivors
When a proposal expires because one recipient never answered or went offline, passing an empty culprit set makes CancelProposal requeue the entry unchanged, including the still-pending member. Because the role counts remain complete, the next LFG update can immediately form the same proposal again and trap the players who did accept in a repeat timeout loop; build the culprit set from pending/offline answers before cancelling the expired proposal.
Useful? React with 👍 / 👎.
| // SECOND live entry. If the first proposal then completed, CreateDungeonGroup put | ||
| // them in a dungeon group while they were still queued for another. | ||
| LFGPlayerStatus const existingStatus = GetPlayerStatus(plr->GetObjectGuid()); | ||
| if (existingStatus.state == LFG_STATE_PROPOSAL) |
There was a problem hiding this comment.
Reject requeues from merged queued members
This guard only rejects LFG_STATE_PROPOSAL, but after a solo player has been absorbed into another queue entry, m_playerData no longer has an entry under their own GUID while m_playerStatusMap still reports LFG_STATE_QUEUED. If that player sends another CMSG_LFG_JOIN to change roles or dungeons, currentInfo misses and the solo branch creates a second live queue entry while the merged entry still lists them, so the duplicate check needs to use FindQueueEntryContaining or reject queued status as well.
Useful? React with 👍 / 👎.
| for (playerGroupMap::const_iterator it = proposal.groups.begin(); | ||
| it != proposal.groups.end(); ++it) |
There was a problem hiding this comment.
Send proposals after all participants are recorded
The new proposal packet serializes proposal.groups, but SendDungeonProposal still sends each player's initial proposal inside the loop that is populating groups and answers. For any multi-player match, everyone except the last loop iteration receives an opening proposal that omits later participants, so the popup can show an incomplete group until someone answers; build the full proposal first, then send it to every participant.
Useful? React with 👍 / 👎.
| SendLfgUpdate(*bad, GetPlayerStatus(*bad), false); | ||
|
|
||
| m_queueSet.erase(*bad); | ||
| m_playerData.erase(*bad); |
There was a problem hiding this comment.
Avoid erasing the queue entry behind CancelProposal
When the declining culprit is also proposal.queueGuid (for example the solo player whose entry absorbed the match, or a debug-solo proposal), this erase destroys the map node that entry still points to, and the function then reads entry->currentRoles.empty() below. That makes an ordinary proposal decline a use-after-free on the world thread; delay erasing proposal.queueGuid until after the survivor check, or stop using the cached pointer after this erase.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Now that roleCheck is a persistent reference and CMSG_LFG_SET_ROLES reaches this path, a party member who was not in the original currentRoles map (for example, someone invited after the role check started, or a crafted client) is inserted by the unchecked currentRoles[plrGuid] = roles below. That lets a player who never passed JoinLFG eligibility/lock checks get copied into queueInfo->currentRoles and proceed to the proposal, or abort the original party by overfilling the quota; check that plrGuid already exists in the role check before accepting the reply.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (*bad == proposal.queueGuid) | ||
| { | ||
| continue; // handled below, after `entry` is finished with |
There was a problem hiding this comment.
Re-key survivors when the queue owner declines
When the player whose GUID is proposal.queueGuid declines while other members remain in entry (for example, a solo entry that absorbed another solo or party), this continue leaves m_playerData[proposal.queueGuid] and m_queueSet keyed by a player who was just removed from currentRoles. If that player queues or leaves again, FindQueueEntryContaining hits their own key first and JoinLFG can overwrite the survivor entry, silently dropping the players who were returned to the queue; move the survivor entry to a remaining participant/group key before requeueing it.
Useful? React with 👍 / 👎.
| // in LeaveLFG, not a reason to cancel the wrong thing. | ||
| Group* pGroup = plr->GetGroup(); | ||
|
|
||
| sLFGMgr.LeaveLFG(plr, pGroup != nullptr); |
There was a problem hiding this comment.
Require party leadership before leaving as a group
With this call, any grouped player who sends CMSG_LFG_LEAVE takes the isGroup path, and LeaveLFG then iterates every member and erases the group's queue/role-check state without checking IsLeader. A non-leader using a modified client can therefore cancel the whole party's dungeon-finder queue or active role check; keep the grouped path limited to the party leader, or only remove the caller when they are not allowed to act for the group.
Useful? React with 👍 / 👎.
| if (ans->second != LFG_ANSWER_AGREE || !sObjectAccessor.FindPlayer(ans->first)) | ||
| { | ||
| silent.insert(ans->first); |
There was a problem hiding this comment.
Remove timed-out premades together
When a proposal expires because one member of a premade party never answers or goes offline, this builds the culprit set with only that player's GUID, unlike the decline path which expands to every member in the same proposal.groups entry. Fresh evidence is that the current timeout code calls CancelProposal directly with silent, so the responding party members are requeued without their party mate and can later be pulled into a dungeon as if only part of the premade had queued.
Useful? React with 👍 / 👎.
The dungeon finder's matching spine could not form a group under any input. Five independent defects, each sufficient on its own: Role needs were gated on `dungeon->DifficultyID == DUNGEON_DIFFICULTY_NORMAL`, comparing a RAW client DifficultyID against the internal 0-based enum. No queueable row in LfgDungeons.dbc carries DifficultyID 0, so the branch never fired and every entry reported needing nobody. RoleMapsAreCompatible then computed (3-0)+(3-0) = 6 > 3 and refused every pair, including two solos. Take the composition from the dungeon's own row -- Count_tank, Count_healer, Count_damage -- which removes the difficulty translation from this path and covers the 108 of 247 queueable TypeID 1 rows that are not 1/1/3 five-mans (scenarios 0/0/3, solo content 0/0/1, raid finder 2/6/17, flex 0/0/25). The role mask is a BITMASK, not an enum. The client's LFD frame has four independent checkboxes, so a player offering tank-or-damage sends 0x0A -- observed on the wire in capture-000112 seq 90341. Every consumer switched on the exact values 0x02/0x04/0x08, so a hybrid counted as zero of everything: solo hybrids merged into a full-size entry that still reported every role missing and could neither complete nor merge again, and a premade containing one hybrid failed its role check outright. Resolve the mask by backtracking assignment instead; greedy mis-assigns, because handing the tank slot to a tank-or-healer player can strand a tank-only specialist. `neededTanks = 1 - tankCount` in a uint8 wrapped to 255 for a two-tank party, which the old arithmetic then read as -254 and passed, merging parties that could never complete. The resolver counts down from the quota and cannot underflow. Completion was only ever tested inside MergeGroups, so a premade of exactly five with a correct composition -- the commonest premade case -- was never merged with anything and never proposed. Test it wherever an entry becomes eligible, and dequeue on proposal: without that the entry stays QUEUED, gets matched again next tick and fires a fresh proposal every tick forever. Also fixed, all reachable the moment Update() ticks: - RemoveOldRoleChecks erased inside a `++it` loop over an unordered_map. It is the first thing Update() calls. - LFG_TIME_ROLECHECK was 45*IN_MILLISECONDS added to a seconds-domain time_t, expiring role checks after 12.5 hours instead of 45 seconds. - MergeGroups erased the absorbed entry from m_playerData but not m_queueSet, leaving a stale queue entry that could give one player two live proposals. - Both matching loops iterated m_queueSet while merges erased from it. - The role check was stored before the loop that fills currentRoles ran, so it listed nobody and PerformRoleCheck saw "everyone" answer on the first reply. - PerformRoleCheck mutated a COPY of the stored role check, so no member's answer was ever recorded and a party of two or more could never finish. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reply half of the LFG role check had no handler and no registration, so
the client's answer was dropped at the dispatcher without a log line and a
party entered LFG_STATE_ROLECHECK and stayed there permanently.
Body derived from the client's own writer sub_6688D0, reached as vtable slot 1
behind the opcode thunk sub_6615FE (which writes 2210):
sub_40F075(pkt, *(uint32*)(this + 16)); // WriteUInt32 -- role mask
sub_40F018(pkt, *(uint8 *)(this + 20)); // WriteUInt8 -- role check counter
Flat -- no bit packing and no GUID, so nothing to XOR or reorder. All 99
build-18414 packets in the corpus are exactly 5 bytes, which agrees.
Note the Lua SetLFGRoles() does not send this; it only mutates local state.
The packet is emitted by CompleteLFGRoleCheck, i.e. on confirmation.
Fixture uses real captured bodies, not inverses of our own reader:
capture-000086 seq 16621 08 00 00 00 00 damage only
capture-000112 seq 90341 0A 00 00 00 00 TANK|DAMAGE
The second is why the mask must be treated as a bitmask: it is one player
offering either role, and it is what the previous exact-value matching threw
away. Corpus catalogueGenerationId 2BE10C89...88752.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LFGProposal had no member initialisers, and groupRawGuid/groupLeaderGuid are the only two scalars SendDungeonProposal does not always assign -- it sets them solely on the premade path. Yet it READS groupRawGuid to decide whether to set it, and CreateDungeonGroup branches on it to choose between reusing an existing group and making a new one. An all-solo proposal therefore picked its branch from whatever was on the stack. LFGPlayers had the same problem for joinedTime and the three needed* counts, which decide both completion and what the queue advertises. SendLfgProposalUpdate dereferenced three find() results without checking end(). It is reachable, not theoretical: SendDungeonProposal skips offline players when filling `groups` and `answers` but still lists them in `currentRoles`, so a player who queues, logs out and logs back in arrives with no entry of their own and crashes the session the moment another member answers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The role check prompt never appeared on the client however correct the server state was: the outbound packet was still the 3.3.5 shape -- uint32 state, flat counts, raw uint64 GUIDs -- and shared no field order with 18414. It was also unadmitted, so it never left the server at all. Derivation. The CMSG technique this campaign runs on (opcode thunk -> vtable slot 1 -> body writer) does not apply to SMSG: there is no opcode thunk because the client never sends these. Working from the other end instead: - GetLFGRoleUpdate binds to sub_98C59C -> sub_98C4D2, which reads a global block and yields inProgress = (state == 2), a slot count, a member count, a category derived from slot[0] & 0xFFFFF, and a battleground GUID. - The applier at 0x98953C fills that block from a parsed struct, giving the field set and types: state at +0x10, slot vector at +0x14/+0x18, member vector at +0x24/+0x28 with stride 0x18, GUID at +0x38. - The wire reader itself sits in a third layer of generated code reached indirectly, with the opcode nowhere in the image as a literal, so field ORDER had to come from traffic rather than from a reader. So the order is a hypothesis verified against real bytes, not read off a writer. It decodes two captures of deliberately different shape to zero leftover, and the writer added here reproduces both byte for byte: capture-000075 seq 891708, 35 B: partyIndex 0, 2 members, dungeon type 1 capture-000059 seq 719547, 68 B: partyIndex 1, 5 members, dungeon type 6 Corpus catalogueGenerationId 2BE10C89...88752. Two things the reference layout this was checked against gets wrong: partyIndex is not always zero -- the second capture carries 1 -- and the leader's entry must come first, which both captures confirm by carrying the LEADER bit on member 0 while later members are still zero. Also fixes an unchecked find() in the sender: a role check whose leader had already left dereferenced end(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last packet standing between a formed group and a client that can see it. The body was the 3.3.5 shape -- flat fields and a per-player run of single bytes -- and shared no field order with 18414. It was also unadmitted. Derived the same way as the role check: the client's wire reader sits in generated code reached indirectly with the opcode nowhere in the image as a literal, so field order comes from traffic rather than from a reader, and is therefore a hypothesis that had to be verified rather than trusted. Verified byte-exact against two captures chosen to differ as much as the corpus allows, both decoding to zero leftover, and the writer reproduces both: capture-000044 seq 1948, 64 B: 5 players, roles 0x03/0x04/0x08 x3 capture-000059 seq 2063424, 156 B: 25 players, 2 tank / 6 healer / 17 dps The raid case is an independent check on the decode rather than more of the same: 2/6/17 is exactly what LfgDungeons.dbc carries in Count_tank, Count_healer and Count_damage for LFR rows -- a fact established from the DBC, not from this packet. A wrong layout would have to be wrong in a way that happens to reproduce the shipped data. Corpus catalogueGenerationId 2BE10C89...88752. Three corrections to the reference layout the hypothesis came from: - It builds the second GUID as `dungeonEntry | (0x1F45 << 48)`. Real traffic carries neither: the top five bytes are constant 1F 44 00 00 11 across both captures while the low three vary, i.e. a genuine instance-side GUID with a counter, unrelated to the dungeon entry. We do not model that object, so it is sent as zero -- a legal encoding, since every mask bit then reads false and WriteByteSeq emits nothing for a zero byte. - Roles must pass through verbatim. Observed values include 0x32 and 0x09, so bits above DAMAGE are real; masking to the four known role bits would corrupt them. - The recipient is not necessarily player 0. In the raid capture the "is this you" bit sits on entry 6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two pieces, and together they close the loop: a proposal can now be answered, and the matchmaker can now run at all. CMSG_LFG_PROPOSAL_RESPONSE had no handler and no registration, so accept and decline both did nothing -- the reply was dropped at the dispatcher. Body derived from the client's own writer sub_66A29E (vtable slot 1 behind the opcode thunk sub_6622E8, which writes 7581), with GUID A at this+24..31 and GUID B at this+48..55: uint32 proposalId, clientQueueId, flags, joinTime bits accept, mask A6 A0 A2 A4 B6 B7 A3 B4 A7 B1 A5 B0 A1 B2 B3 B5 Flush bytes A3 A6 A4 A1 B7 B0 A7 B6 A5 B3 B1 B5 B4 A0 A2 B2, XOR 1 when present Fixture is capture-000059 seq 2063770, and it is worth more than a size check: it is the client's answer to seq 2063424 in the SAME capture, the 156-byte SMSG_LFG_PROPOSAL_UPDATE derived in the previous commit. Every echoed field matches -- proposal 11132, queue 37743, flags 3, join time 1409232359, and both GUIDs. The inbound and outbound layouts were derived separately and agree, which neither could establish on its own. Nothing in the body is authority. The server answers on behalf of the CALLER and keys on its own proposal id, so a client returning someone else's guidA cannot answer for them. The WUPDATE_LFGMGR timer was configured at startup but never consumed, so LFGMgr::Update had no caller anywhere: a player could join the queue and nothing ever looked at it again. This is deliberately the last change of the sequence rather than the first -- the reaper Update calls first erased while iterating, the matchmaker it calls next could not form a group under any input, and the proposal it can now send chose a branch from two uninitialised members. Ticking it before those were fixed would have crashed the world thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit gave LFGMgr::Update a caller for the first time, so this path went from dead code to live code. These defects were all latent behind that; none of them is theoretical now. CreateDungeonGroup, rewritten. Four independent defects on one path: - The leader search looped over every role-flagged member calling Group::Create with no break, so two merged premades carrying two LEADER bits ran Create twice on one object. Each call does its own GenerateGroupLowGuid and its own INSERT INTO groups in its own transaction, orphaning the first group id and stranding that id's group_member rows. - If a leader bit was set but every leader-flagged player was offline, Create never ran while AddMember still did, building a group with id 0 and an empty leader guid and inserting it into m_groupSet. - The existing-group branch called no AddMember at all. One premade plus solo queuers is the commonest LFD composition, and those solos were dequeued, told a group had been found, and never put in one. - Nothing called sObjectMgr.AddGroup, so GetGroupById could not find the group, it leaked at shutdown, and the boot path called RemoveGroup on a group that had never been added. It also no longer calls SetDungeonDifficulty(Difficulty(dungeon->DifficultyID)). That mixes the raw client key with the internal 0-based enum, making every normal five-man heroic, and GetBoundInstances indexes m_boundInstances by it unchecked while MAX_DIFFICULTY is 4 -- raw ids on LFR, scenario and flex rows reach 14. Leaving the existing difficulty is wrong-but-safe; setting a wrong one is neither. IsProposalSameGroup skipped ungrouped players entirely, so a two-man party matched with three solos returned true. The proposal was then treated as a premade and reused the party's group without adding the solos. It also returned true when nobody was grouped at all. ProposalUpdate now returns immediately on a decline. Falling through carried two bugs at once: ProposalDeclined can erase the proposal from m_proposalMap, leaving the code below iterating and writing through a dangling pointer; and when it does not erase, it removes the decliner from `answers`, so four accepts plus one decline in a five-man read as unanimous, built a FOUR-man group and teleported it in. GetDungeonFinderRewards was dereferenced unconditionally. dungeonfinder_rewards ships 66 rows covering levels 15-80, so every level 81-90 character -- every MoP-relevant one -- crashed the world server on a tracked boss kill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both reachable today -- CMSG_LFG_JOIN and CMSG_LFG_LEAVE are already live. GetJoinResult ended its solo branch with an unconditional `result = ERR_LFG_OK`, discarding every check above it. A solo player with Dungeon Deserter, on LFG cooldown, in a battleground or in an arena was always admitted. The level 15 minimum was worse: that test existed only inside the group branch, so a solo player below 15 was never checked at all. In the group branch, `result` was assigned per member including an else-OK, so only the LAST iterated member's verdict survived -- a party containing one deserter was admitted whenever the last member happened to be clean. `LfgJoinResult result;` was also read uninitialised when a group had members but every getSource() returned null. HandleLfgLeaveOpcode tested `pGroup && pGroup->IsLeader(...)`, so a non-leader went down the SOLO branch. That branch erases m_playerData[playerGuid], and for a grouped queuer no such entry exists: the party's real entry, keyed by the group guid, stayed in the queue untouched while the client was told it had left. Whether a non-leader may cancel for the party is a permission question, and it belongs in LeaveLFG rather than being answered by cancelling the wrong thing. This one was mine, from the commit that first wired the opcode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-model review (Devin SWE-1.7 Max) returned BLOCK on seven findings. All seven were real. Two risks I flagged came back clean: sending the proposal's second GUID as zero is safe (the client echoes it and never surfaces it), and the TryFormGroup/FindQueueMatches snapshot iteration is sound. The resolver could hang the world thread. My own comment claimed "at most 5 players and 3 roles, bounded by 3^5" -- but this is not a five-man-only path. Raid finder rows ask for 2/6/17 and flexible raid for 0/0/25, and I had derived a 25-player LFR proposal in the same session that produced that comment. Measured on the exact algorithm in isolation, 26 hybrid players competing for 25 slots: naive 127,337,429 calls 545.014 ms memoised 934 calls 0.054 ms 545 ms of world-thread time, growing exponentially with player count. Keying dead ends on (index, remaining quota) bounds the search by (players+1) x (tank+1) x (healer+1) x (damage+1). Every fit/no-fit result is identical between the two, so this changes cost and not semantics. Declines. Returning early fixed the use-after-free but not the rest. It left the other members holding a proposal window that never closed, and the stale proposal stayed in m_proposalMap on the non-premade path -- where a later accept could still complete it short, which is the bug the early return was supposed to prevent. A decline now marks the proposal failed, sends SMSG_LFG_PROPOSAL_UPDATE to everyone still listed so their windows close, runs the per-player teardown, and erases the proposal unconditionally. ProposalDeclined no longer erases it or prunes members out of the maps -- the caller owns the proposal, and pruning was what let survivors read as unanimous. CreateDungeonGroup leaked a Group on the unknown-dungeon return: the lookup sat after creation, so it returned having already new'd a Group, run Create (a group id plus an INSERT INTO groups) and registered it with ObjectMgr. The lookup now happens first. Detaching a player from a two-man group makes Group::RemoveMember call Disband, which neither unregisters nor deletes the object. Both detach sites now use Player::RemoveFromGroup, the codebase's own helper for this, which handles RemoveGroup and delete. Re-queuing during a live proposal produced a second queue entry: the duplicate cleanup in JoinLFG is guarded on existing queue data, and TryFormGroup erases that the moment a proposal is sent. A player sitting on an open proposal is now refused. Both new parsers now require the body to be fully consumed. Unread tail data is the cheapest signal that a body was read wrongly and must not be swallowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-caught while anticipating the re-review, and it is a regression from the previous commit rather than a pre-existing bug. ProposalDeclined calls LeaveLFG for the DECLINER alone. The other members were left at LFG_STATE_PROPOSAL: their queue entry was already erased when the proposal fired, nothing else resets them, and the re-queue guard added in the same commit now refuses a player in that state. So one decline would have locked every other member out of the dungeon finder until they relogged -- turning a fix for a duplicate-entry bug into a worse denial. Membership is snapshotted before ProposalDeclined runs, because that path calls LeaveLFG and can mutate the maps being walked. Everyone leaving LFG on a decline is deliberate. Retail requeues the non-decliners, but their queue data is gone by this point and rebuilding it is separate work. Leaving cleanly is correct-but-less, and it is visible to the player rather than silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From the focused re-review. Clearing server-side state and closing the proposal window is not enough on its own: without an explicit LEAVE the client keeps showing itself queued for a queue that no longer exists. Sent before the status entry is erased, since the update is built from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lure
The client states the three outcomes of a failed proposal plainly, and the
previous behaviour matched none of them:
ERR_LFG_PROPOSAL_FAILED "Someone has declined the invite. You have
been returned to the front of the queue."
ERR_LFG_PROPOSAL_DECLINED_SELF "You have been removed from the queue
because you did not accept the invitation."
ERR_LFG_PROPOSAL_DECLINED_PARTY "...because someone in your party did not
accept the invitation."
So the decliner leaves, their premade leaves with them, and EVERYONE ELSE goes
back in the queue. The previous commit ejected all of them, which I had
described as correct-but-less; it is simply wrong.
That required a change of shape. TryFormGroup used to erase the queue entry the
moment a proposal fired, leaving nothing to put people back into. It now removes
the entry from the match set and marks it LFG_STATE_PROPOSAL while keeping the
data, so a cancellation can restore it. CancelProposal implements the three
outcomes; the decline path routes through it.
That also fixes, from the PR review:
- No proposal timeout existed. A recipient who ignored the popup, disconnected,
or whose client-side timer lapsed left everyone else pinned at
LFG_STATE_PROPOSAL for ever, and JoinLFG refuses that state, so they could not
re-queue until relog. RemoveOldProposals now reaps them through the same
cancellation path, which requeues the survivors.
- Any logged-in player could cancel someone else's proposal. m_proposalId is a
plain incrementing counter, so an id is trivially guessable, and writing to
proposal->answers INSERTED the caller -- a `false` answer from a stranger
cancelled a group they had nothing to do with. Only participants may answer.
- A queued player who logged out was skipped when filling `groups` and `answers`
while still counted in `currentRoles`, so the online members could all accept,
allOkay saw no pending answer for the absent one, and a SHORT group was built
and teleported. Offline members are now dropped from the entry before a
proposal is sent, and the entry goes back to looking.
- A raid-sized dungeon built a normal party. Group::IsFull caps at
MAX_GROUP_SIZE and AddMember just returns false past it, so a raid finder
proposal (2/6/17 = 25) completed as a five-man while the other twenty were
told a group had been found, never added and never teleported. Groups whose
dungeon quota exceeds a party are converted to raid before members are added,
and the AddMember return is no longer discarded.
Not fixed here, and worth stating: retail also displays those three messages,
but they are delivered through SMSG_DISPLAY_GAME_ERROR, which has no sender
anywhere in this tree and whose body is not yet derived. The behaviour is right;
the notification text is still missing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r people
Mirrors `.debug bg`, which lets a battleground start 1v0. The dungeon finder has
the same problem and worse: a normal five-man will not form until 1 tank,
1 healer and 3 damage are all present, so on a test realm with two accounts the
proposal, group-creation and teleport paths are simply unreachable -- correct
behaviour that cannot be exercised.
.debug dungeon a game master's queue entry completes on its own
.debug dungeon group as above, and it also absorbs whoever else is waiting,
whatever roles they picked
.debug dungeon off back to normal matchmaking
Bare `.debug dungeon` toggles off when a mode is already active, matching how
`.debug bg` behaves with no arguments.
While any mode is active a game master leads the resulting dungeon group
regardless of who holds the LEADER bit, so the operator keeps control of the
group under test.
Every relaxation is gated on the queue entry actually CONTAINING a game master:
- TryFormGroup waives the needed-role test only for such an entry.
- RoleMapsAreCompatible waives only the role composition in group mode, and only
when a GM is on one side. The size cap and the duplicate-membership check both
still apply, so a five-man still caps at five and nobody can end up in two
entries.
Scoping it this way matters. Relaxing the matchmaker globally would change how
ordinary players match each other while the operator is testing, which makes a
debug switch untrustworthy -- you can no longer tell whether what you observed
was the system working or the switch lying.
Game master is account security, not `.gm on`: the operator should not have to
make themselves untargetable to test the dungeon finder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both flagged functions were already large on master (ProposalUpdate 100 lines, SendDungeonProposal 95); my changes grew them to 143 and 106. Fair feedback, so fixed rather than waived. The decline branch is lifted out of ProposalUpdate into DeclineProposal. It was a self-contained forty lines that answered one question -- who is responsible for this cancellation -- and reads better named than inline. ProposalUpdate is back to 113 lines. SendDungeonProposal now takes the queue guid instead of recovering it by scanning m_playerData for an entry whose value has the same ADDRESS as the LFGPlayers* it was handed. The caller already knows the key. Beyond the complexity, identifying a map entry by the address of its value is the sort of thing that quietly stops working the first time anyone copies the struct, and LFGPlayers is copied in several places already. No behaviour change; 115/115 still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by driving two clients: two solo players queued for the same dungeon, both damage. One saw a full panel -- 0/1 tank, 0/1 healer, 2/3 damage, average wait, a live time-in-queue. The other saw a stub: no role counts, no average wait, a placeholder "< 1 minute", and most of the minimap eye's tooltip missing. The 2/3 proves the merge itself was fine. Both packets that describe a queue were reporting it to the wrong identity. SMSG_LFG_QUEUE_STATUS stamped every recipient with the merged entry's KEY, which is whichever entry did the absorbing. The absorbed player joined under their own guid -- that is what SMSG_LFG_UPDATE_STATUS sent them as requesterGuid -- so a status arriving under a stranger's identity does not match the queue their client is tracking and is ignored. The absorbing player never saw this, because for them the merged key IS their own guid, which is exactly why this looked like "one client works and the other does not". SMSG_LFG_UPDATE_STATUS had the mirror image. GetStatusPacketData looked the player up by queue guid alone, and a merged solo queuer has no entry of their own -- MergeGroups folds them into the absorber and erases theirs. The lookup missed, the caller got a default-constructed struct, and the update went out with zero roles, zero needed counts and a zero join time. It now falls back to whichever entry actually lists the player. Neither of these is visible from the corpus: it proves what a retail server sent, not that ours addressed the right person. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported from a live client: on the absorbed player, Leave Queue did nothing. It was worse than nothing. LeaveLFG erased m_playerData and m_queueSet by the player's OWN guid, but a solo queuer who has already been merged has no data under that key -- MergeGroups folds them into the absorbing entry and erases theirs. So the erase was a no-op while the client was still sent LFG_UPDATE_LEAVE: the UI cleared and the server kept them queued inside the merged entry, where a later proposal would have pulled them into a dungeon they had left. This is the third bug from one root cause, after the queue-status and update-status identity bugs in the previous commit. Anything keyed on a player's own guid silently misses them once they have been merged. So the lookup is now a named helper, FindQueueEntryContaining, and both the leave path and GetStatusPacketData go through it rather than each open-coding the scan. Removing a player also recomputes the entry's needed roles -- the survivors need one more of whatever the leaver was covering -- and drops the entry entirely when the last member leaves. The group leave path had the same hazard per member and now uses the same helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found live: after one successful proposal the player could not queue again -- five CMSG_LFG_JOIN attempts in ninety seconds, all refused. My regression, from the commit that made TryFormGroup keep the queue entry alive so a declined or timed-out proposal could put the survivors back. That commit handled both failure paths and neither success path: on success nothing erased the entry, so it sat in m_playerData for ever with currentState LFG_STATE_PROPOSAL, every member's stored status stayed LFG_STATE_PROPOSAL, and the re-queue guard added in the same commit refuses exactly that state. Enter a dungeon once, never queue again until relog. The success path now tears the entry down and moves each member to LFG_STATE_IN_DUNGEON, including any whose teleport was denied -- they must not be left reading LFG_STATE_PROPOSAL either. The guard itself was also too trusting. LFG_STATE_PROPOSAL is written in several places and cleared in fewer, so any path that forgets to reset it locks the player out of the dungeon finder entirely. It now asks m_proposalMap whether a proposal is actually awaiting this player's answer, which cannot go stale: if no live proposal lists them, there is nothing to protect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ects From the second round of PR review. BLOCKING -- CancelProposal was a use-after-free on the world thread. It caches `entry = GetPlayerOrPartyData(proposal.queueGuid)`, then erases m_playerData for each culprit, then reads entry->currentRoles.empty(). A culprit is very often the entry key itself: the solo player whose entry did the absorbing, or the single queuer in a `.debug dungeon` proposal. Erasing m_playerData[queueGuid] destroyed the node `entry` pointed into and the survivor check read it. So an ordinary decline -- on exactly the path currently being live-tested -- was undefined behaviour. The queue entry is now left alone inside that loop and handled after `entry` is finished with. The timeout reaper cancelled with an EMPTY culprit set, which requeued the entry unchanged, including the member who never answered. The role counts were still complete, so the next tick re-formed the same proposal and timed out again, trapping everyone who did accept in a permanent loop. Whoever failed to answer, or went offline, is now the culprit -- exactly as a decliner is. A merged solo queuer re-joining created a SECOND live entry. The duplicate cleanup keys on m_playerData under the player's own guid, which an absorbed player does not have, so the cleanup was skipped and the solo branch built a fresh entry while the merged one still listed them. It now resolves through FindQueueEntryContaining and removes them from whatever entry actually holds them. SendDungeonProposal sent each player's opening proposal from INSIDE the loop that fills `groups` and `answers`. The packet serialises those maps, so every recipient but the last saw a proposal missing the members added after them -- the ready popup showed an incomplete group until somebody answered. Built first, sent second. A member who accepted and then logged out before the final answer still passed allOkay, and skipping them built a short group and teleported it while groupStatus recorded a role for someone never added. That now cancels, with the absent member as the culprit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by instrumenting a live queue. The diagnostic said it outright:
LFG JoinLFG: solo entry for Humanwarrior stores dungeons={6}
LFG SendDungeonProposal: entry dungeons={6} -> chose 6 (entry 0x0100000C)
LFG TeleportToDungeon: Humanwarrior DENIED, dungeon 12 map 349, player error 6
The queue entry was correct throughout -- the player asked for dungeon 6 and we
chose dungeon 6. GetDungeonEntry(6) still returned 0x0100000C, which is id 12.
LfgDungeonsEntryfmt began with 'i' rather than 'n'. DBC_FF_IND is 'n' and marks
the INDEX column; DBC_FF_INT is 'i' and is just a uint32. With no index column
declared, DBCFileLoader::GetFormatRecordSize returns index_pos -1 and
AutoProduceData falls into its fallback branch, numbering indexTable 0..
recordCount-1 in FILE ORDER. Every LookupEntry on this store was therefore
positional.
Reading the shipped DBC confirms the observed values exactly:
row 6 -> id 12 "Stormwind Stockade" (so LookupEntry(6) gave Stockade)
row 12 -> id 26 "Maraudon ...", MapID 349
which is the whole chain: LookupEntry(6) yields id 12, CreateDungeonGroup passes
that 12 to TeleportToDungeon, LookupEntry(12) yields map 349. A player queueing
Deadmines was offered Stockade and then denied a teleport to Maraudon.
The second consequence is quieter and larger. Without an index column
GetNumRows() reports recordCount (343) instead of maxId+1 (776), so every loop
of the form `for (id = 0; id < GetNumRows(); ++id) LookupEntry(id)` -- three of
them in the LFG code -- walked less than half the id space, and mis-identified
every row it did visit. That is why a level-appropriate character came back with
a 329-entry locked-dungeon list.
Of the 129 format strings in DBCfmt.h, 86 start with 'n', 29 with 'd', 10 with
'x' and only 4 with 'i'. Two of those four (AreaTable, ScalingStatValues) carry
an 'n' later in the string and are properly indexed. Of the remaining two,
ChatChannels is genuinely unindexed and its only consumer knows it -- it linear
scans comparing ch->ID, with a comment saying so. LfgDungeons was the only store
looked up by ID while indexed by row.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ED correctly The packet log printed "OPCODE: UNKNOWN (0x1E3B)" during a live test. SMSG_LFG_PROPOSAL_UPDATE and SMSG_LFG_ROLE_CHECK_UPDATE transmit correctly -- they are admitted by IsEnterWorldConverted, which is the real send gate -- but neither had a DefS row, so the logger could not name them. DefS is logging metadata only in this tree and does not affect delivery. SMSG_LFG_TELEPORT_DENIED wrote a uint32. Every 18414 capture of it in the corpus is exactly 1 byte: capture-000044 seq 70879 and 219256, capture-000465 seq 283035, capture-000628 seq 31349, capture-000873 seq 154730. Now a uint8. It stays UNADMITTED on purpose. The size is settled but the value space is not -- the captured body carries 0x10 (16) while our LFGTeleportError enum stops at 8, so our codes are provably not the client's. A correctly sized packet with a wrong code shows the player a confidently wrong reason, which is worse than the current silence. This is why an LFG teleport failure currently produces no on-screen message at all: the packet is built, logged, and dropped at the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rything
Reported from a live client: every dungeon in the game appeared in the finder,
so a player could queue for content they cannot enter.
The cause is ours. FrameXML's LFGList_DefaultFilterFunction shows a dungeon when
`not LFGLockList[dungeonID]`, and LFGLockList is built from the lock array in
SMSG_LFG_PLAYER_INFO -- which we sent EMPTY. An empty array does not mean "no
locks are known", it means "nothing is locked", so the client correctly
concluded every dungeon was available. The eligibility filter was not degraded;
it was absent, because we never gave it anything to filter on.
Layout derived from a real reply rather than a fork: capture-000006 seq 1953,
6068 bytes to a max-level character, decoding as lockCount 206, hasPlayerGuid 0,
randomDungeonCount 35.
bits WriteBits(lockCount, 20)
WriteBit(hasPlayerGuid)
WriteBits(randomDungeonCount, 17)
FlushBits -> 38 bits, 5 bytes
...random dungeon reward records, variable length...
tail lockCount x 16 bytes, flat and unpacked:
uint32 dungeonEntry (TypeID << 24) | id
uint32 lockStatus
uint32 subReason1
uint32 subReason2
That the array sits at the TAIL is what makes this shippable now. With zero
random records the header and the array are adjacent, and the client installs
the list and raises LFG_LOCK_INFO_RECEIVED whether or not random rows follow --
so the reward plumbing this manager cannot express is not needed to make the
filter work. The random count stays 0 and is a separate piece of work.
No translation is needed in either direction, which is worth stating because it
looks too convenient: FindRandomDungeonsNotForPlayer already returns a map keyed
by LfgDungeonsEntry::Entry(), and that IS the wire's dungeonEntry field; its
LFGForbiddenTypes values are the client's LFG_INSTANCE_INVALID_CODES verbatim.
The reference packet's own distribution confirms the codes line up -- 167 of its
206 records carry 3, LEVEL_TOO_HIGH, which is exactly what a max-level character
sees for low-level content.
subReason1 and subReason2 stay zero. They carry the required and current item
level for the gear-score reasons, where the client formats them as
"Requires: %2$d. Currently %3$d."; all 206 records of the reference capture have
them zero, and this manager does not compute a gear score for the lock list.
Sent only in reply to CMSG_LFG_LOCK_INFO_REQUEST, never pushed at login. The two
pair seven-for-seven in capture-000006, and the client asks at world-enter --
CMSG_LFG_GET_STATUS then CMSG_LFG_LOCK_INFO_REQUEST at adjacent sequence
numbers. Our handler was already registered and already replied in the right
place; only the content was missing.
Fixture uses the reference packet's own first five lock records byte for byte,
including a TypeID 2 raid entry so the array is not assumed to be dungeons-only,
plus its exact 5-byte header to pin the 20/1/17 field widths and their order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing this branch onto merged master brought master's difficulty translation
into CreateDungeonGroup, and that block carries its own lookups of `dungeon` and
`groupGuid`. This branch already has both, earlier and deliberately:
* `dungeon` is looked up at the top of the function, BEFORE anything is
created, because the original order returned on an unknown id having already
new'd a Group, run Create (a group id plus an INSERT) and registered it with
ObjectMgr -- leaking the object and stranding its rows.
* `groupGuid` is taken once as `ObjectGuid const`.
Keeping both copies was a redefinition and did not compile. The earlier ones are
the right ones to keep, so the later pair is removed and the reason each exists
is noted where it would otherwise look redundant.
Build clean; ctest 110/110.
ba6aeff to
4e5e4c3
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Makes the dungeon finder work end to end. It was broken at every link, and any one of them was fatal on its own.
What was wrong
result = ERR_LFG_OKoverwrote every check; solo deserters, level-10s and players in arenas were all admittedPerformRoleCheckmutated a copy, so no member's answer was ever recordedLFGMgr::Update()had no caller anywhere in the treeGroup::Create, group used beforeCreate, solo queuers never added, never registered with ObjectMgrThe matchmaker deserves detail. Role needs were gated on
dungeon->DifficultyID == DUNGEON_DIFFICULTY_NORMAL, comparing the raw clientDifficultyID against the internal 0-based enum. No queueable row in
LfgDungeons.dbccarries DifficultyID 0, so the branch never fired, every entryreported needing nobody, and
RoleMapsAreCompatiblethen computed(3-0)+(3-0) = 6 > 3and rejected every pair — including two solo queuers.Separately, the role mask is a bitmask, not an enum. The client's LFD frame
has four independent checkboxes, so a player offering tank-or-damage sends
0x0A(observed on the wire in capture-000112 seq 90341). Every consumerswitched on the exact values
0x02/0x04/0x08, so a hybrid counted as zero ofeverything: solo hybrids merged into a full-size entry that still reported every
role missing and could neither complete nor merge again, and any premade
containing one failed its role check outright.
Packet derivation
Four packets. The two CMSGs came off the client's own body writers. The two
SMSGs could not: there is no opcode thunk for packets the client never sends,
the wire readers sit in generated code reached indirectly, and the opcode
literal appears nowhere in the image. So field order came from traffic, which
makes it a hypothesis that had to be verified rather than trusted.
Standard applied: each layout must decode multiple real captures of
deliberately different shape to zero leftover bytes, and the writer must
reproduce those captures byte for byte.
SMSG_LFG_ROLE_CHECK_UPDATE0x12BBSMSG_LFG_PROPOSAL_UPDATE0x1E3BCMSG_LFG_SET_ROLES0x08A2sub_6688D0; fixture capture-000086 seq 16621 and capture-000112 seq 90341CMSG_LFG_PROPOSAL_RESPONSE0x1D9Dsub_66A29E; fixture capture-000059 seq 2063770Two checks that were not arranged and are worth more than the fixtures
themselves:
exactly the LFR quota in
LfgDungeons.dbc(Count_tank,Count_healer,Count_damage), a fact established from the DBC and not from the packet.same capture. Every echoed field matches — proposal 11132, queue 37743, flags
3, join time 1409232359, and both GUIDs. The inbound and outbound layouts were
derived separately and agree, which neither could establish alone.
Three places the reference layout was wrong and the captures said so:
partyIndexis not always 0; the second proposal GUID is not synthesised fromthe dungeon entry; and the recipient is not necessarily player 0.
Verification
captured bytes rather than inverses of our own writers.
catalogueGenerationId 2BE10C89…88752throughout.Known gaps, deliberately not closed here
SMSG_LFG_PLAYER_INFOstill sends the empty shape, so random dungeons arenot offered. Specific dungeons list and queue normally — the client's own
filter treats an empty lock list as "nothing locked". Each random record
carries quest rewards, currencies, call-to-arms entries and an encounter mask,
none of which the legacy manager models; that is a wave of its own.
SMSG_LFG_JOIN_RESULTis not admitted, so a refused join is silent. Thismatters more now that the gate genuinely refuses.
Group_IDinstead ofRandom_ID(id 434 yields anempty set, id 493 over-selects) and carry a placeholder MapID into the
proposal.
Difficulty(DifficultyID)mixes the raw client key with the internal enum, and
GetBoundInstancesindexes
m_boundInstancesby it unchecked whileMAX_DIFFICULTYis 4.CMSG_LFG_TELEPORTremain unregistered.