LFG: make the dungeon finder work end to end, and fix what it uncovered - #82
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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.
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.
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
Enforce group-leave permissions before cancelling parties
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.
Ignore role replies from non-participants
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.
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 👍 / 👎.
Comment on lines
+686
to
+689
| if (!pMember || pGroup->IsMember(it->first)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
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.
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.
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 👍 / 👎.
Comment on lines
+552
to
+553
| for (playerGroupMap::const_iterator it = proposal.groups.begin(); | ||
| it != proposal.groups.end(); ++it) |
There was a problem hiding this comment.
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.
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
Reject role replies from non-participants
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".
Comment on lines
+908
to
+910
| if (*bad == proposal.queueGuid) | ||
| { | ||
| continue; // handled below, after `entry` is finished with |
There was a problem hiding this comment.
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.
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 👍 / 👎.
Comment on lines
+975
to
+977
| if (ans->second != LFG_ANSWER_AGREE || !sObjectAccessor.FindPlayer(ans->first)) | ||
| { | ||
| silent.insert(ans->first); |
There was a problem hiding this comment.
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 👍 / 👎.
MadMaxMangos
force-pushed
the
fix/lfg-matchmaker-role-resolution
branch
from
August 5, 2026 21:04
ba6aeff to
4e5e4c3
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
MadMaxMangos
force-pushed
the
fix/lfg-matchmaker-role-resolution
branch
from
August 5, 2026 21:35
4e5e4c3 to
c1b8c03
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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>
The diagnostics that located the LfgDungeons row-ordinal bug. They print what a
join actually stored and what the proposal actually chose:
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
Read together those three lines are what made the bug obvious: the queue entry was
correct throughout -- the player asked for dungeon 6 and we chose dungeon 6 -- yet
GetDungeonEntry(6) returned 0x0100000C, which is id 12. That is a lookup returning
the Nth ROW rather than the row with that id.
The fix itself is no longer here. It was a one-character change to
LfgDungeonsEntryfmt ('i' -> 'n', the DBC index marker) and it landed with the
instance-difficulty work in PR #81, so rebasing onto that left only the
instrumentation behind. Kept rather than dropped: the finder has a lot of state
between a join and a teleport, and these three lines are what make it legible.
This commit was previously titled for the fix it carried before the rebase.
…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>
/who parsed correctly and matched results, and the server logged "Sent SMSG_WHO with 2 result(s)", but nothing reached the client and nothing appeared in the packet log: four CMSG_WHO in, zero SMSG_WHO out. m_suppressWorldSends stays active for the whole in-world session, and WorldSession::SendPacket drops any opcode IsEnterWorldConverted() does not admit -- before the packet is logged, which is why the send left no trace at all. SMSG_WHO was not on that list. This is the fourth gate in this campaign's own registration checklist: unambiguous value, binary-derived reader, byte-exact reply, AND send-gate admission. The first three were done in 5ba07628b; this is the fourth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fixes. 1. The /who list stayed empty even with the reply on the wire. Our bytes were correct -- decoding our own SMSG_WHO with a reader built from the client's sub_720854 gives name/guild/level/race/class/zone back exactly, zero leftover, and the same reader decodes retail's capture-000135 seq 177672 to "Zynakinka" / "Bratrstvo Oceli" / level 90 / zone 139, also zero leftover. So the layout and the writer were both right and the CONTENT was wrong: we sent virtual realm 0. The client resolves every entry's realm through its realm cache (sub_621E5D against dword_1087180) before it will display the list, and a zero address never resolves. Retail sends 0x03010018 and 0x0304000D in that capture. Send realmID, which is already what Guild.cpp:1039 puts on the wire for the same field. 2. The comment added in the visibility-bookkeeping fix claimed a CMSG_OBJECT_UPDATE_FAILED makes the client abandon the rest of the packet. That is wrong, and IDA settles it: after replying, the client calls sub_79BC10, which walks the update mask, discards the fields and returns 1 -- so the caller's `if (result == 0) break;` does not fire and the loop continues to the next block. The damage is confined to the one object, but it is permanent, because nothing removes the guid from m_clientGUIDs. The fix itself is unaffected; only the explanation was wrong. Dropping the cascade story also removes the need for collateral damage to explain the symptom: the invisible players are exactly the ones the client named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client has been telling us which object it cannot build, and we have been
discarding the message. During tonight's party-phase incident the world log
shows fifteen of these in seventeen seconds:
23:55:29-23:55:46 SESSION: received not handled opcode UNKNOWN (0x1061)
A handler already existed but was never registered, and registering it as it
stood would have been worse than dropping the packet, because its GUID layout
is not this build's. The 18414 writer is the packet class at off_D65304: its
header virtual sub_690E2A writes opcode 4193, and its body virtual sub_694863
emits mask order 3,5,6,0,1,2,7,4 followed by bytes 0,6,5,7,2,1,3,4, each
XOR 1. The old reader used 2,3,5,0,4,7,6,1 / 1,2,5,0,3,4,6,7 -- a different
build's order, carried in from a fork. Replaced with the derived one.
The corpus has no CMSG 4193 at all for 18414, which is the point: a retail
server does not provoke it. Ours does, so the binary is the only oracle here
and this packet is the only signal that names a broken object.
Recovery. m_clientGUIDs is what makes the breakage permanent: once we believe
the client has an object, nothing ever re-sends a create. So the guid is erased
unconditionally, before deciding whether we can do better -- our record is known
false either way. If the object is still on the map we then re-send the create
and re-insert on success.
This is the same bookkeeping bug as the stealth-detection fix, reached from the
other end: that one stopped us recording objects whose create we never sent,
this one repairs records that were true once and are not any more. Together
they should close the "out of phase" party icon, which is not a phase test at
all -- UnitInPhase (sub_8A29C1) is an object-manager lookup, so it means
exactly "I have no object for this member".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client destroys its entire object manager when it processes SMSG_NEW_WORLD.
We never noticed: m_clientGUIDs.clear() has never existed anywhere in this
history. Upstream relies on VisibleNotifier's leftover sweep to remove stale
entries instead, and that works whenever an observer's notifier gets to run on
the old map between one player leaving and the next arriving.
Teleporting a whole LFG party into a dungeon removes that gap. They all leave
within the same tick, so for the FIRST player out no observer ever runs a pass
that misses them, and their guid survives the transition in every other
player's set. On arrival HaveAtClient() is still true, UpdateVisibilityOf takes
the "already at client" branch, and no create is ever sent -- while those
clients have no object at all. Permanently invisible, drawn with the "out of
phase" party icon, because UnitInPhase (sub_8A29C1) is an object-manager lookup
and not a phase comparison.
Measured in world-server_2026-08-07_00-06-06.log, all five players entering
Wailing Caverns at 00:10:31. Entry order was Humanwarrior, Huntdps, Thelma,
Paltank, Gregory, and the erase events are strictly triangular:
Huntdps out of range for players 7, 6, 2, 1
Paltank out of range for players 6, 2, 1
Thelma out of range for players 2, 1
Gregory out of range for player 1
Humanwarrior -- none
Humanwarrior went first, so nothing ever erased him. He is the only player with
no create in the instance, and at 00:15:32 all four other clients reported
CMSG_OBJECT_UPDATE_FAILED for exactly his guid -- which is only visible at all
because the previous commit registered that handler. He could see everyone,
because his own set had been cleaned by the reciprocal path on the way out.
Clearing on the worldport ack is the correct point: the ack answers the
SMSG_NEW_WORLD the client has already acted on, and every failure path below
re-teleports and so triggers another wipe anyway.
m_pendingEmoteRefresh gets the same treatment for the same reason -- those are
queued value re-sends aimed at objects the client has just discarded, and
delivering one would only produce a spurious update-failed report.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CMSG_LFG_TELEPORT is a voluntary per-player action, but the `in` branch called TeleportToDungeon for the whole group. The comment justified that by arguing the map check inside would filter it down -- everyone else is already inside, so only the member who left still qualifies. That assumption fails as soon as the rest of the group is outside too. With the whole party in the world every member passes the map check, so one player's eyeball drags all five in. Reported live: ".tele group northshire", then any single member -- not just the leader -- clicking Teleport to dungeon moved the entire group. TeleportToDungeon now takes an optional onlyPlayer. The destination is still resolved from the group, which is what puts a returning player back with the party rather than at the entrance, and all the per-player refusals and denied replies are unchanged; only the set of members moved is restricted. CreateDungeonGroup passes no onlyPlayer and still moves everyone, which is correct -- a proposal accept IS a group entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 18414 create validator tests all nine speeds for approximate equality with
zero and refuses the object if any of them is nearer than 2.3841858e-7 (2^-22).
The refusal is the expensive kind: sub_768D2F fails, sub_769816 fails,
sub_79DC30 returns 0, and the block loop in sub_79E087 BREAKS -- so the object
is lost AND so is every later block in the same packet, with no reply of any
kind. Combined with m_clientGUIDs being populated per block BUILT rather than
per block delivered, one bad object silently poisons the client's view of every
object behind it in that packet.
Zero speeds are neither hypothetical nor always bad data. A totem SHOULD have
SpeedWalk 0 -- it does not walk -- and Unit::UpdateSpeed multiplies straight
through GetCreatureInfo()->SpeedWalk (UnitSpeed.cpp:246) with no validation
anywhere; a grep for SpeedWalk across src/ returns only the struct field and
that multiply. 64 rows in creature_template have a non-positive walk or run
speed, and these are not obscure:
3968 Sentry Totem, 5923 Poison Cleansing Totem, 5924 Cleansing Totem,
5926 Frost Resistance Totem, 7467 Nature Resistance Totem,
15803 Tranquil Air Totem, 17539 Totem of Wrath, 30527 Training Dummy
all carry SpeedWalk 0, plus 55151 Rumpus Brute (-3.72738e-21), 61928 Sik'thik
Guardian (-2.97773e-20) and 57421 Mothran (SpeedRun 3.08858e-30). Any player
gaining sight of a shaman totem lost the remainder of that update packet.
SetSpeedRate clamps a negative rate to exactly 0.0f (UnitSpeed.cpp:284) and
UpdateSpeed's min_speed floor is 0 without SPELL_AURA_MOD_MINIMUM_SPEED, so a
-100% snare produces a true zero by the same route.
Clamped at the create writer rather than in UpdateSpeed on purpose: the unit
keeps its real speed rate and still does not move, the data stays as authored,
and no future producer can bypass the correction, because this is the only
place the create block's speeds are filled in.
Found by adversarial review of the create path after the far-teleport fix
(298f184da) had already explained the reported symptom. This is a separate,
latent defect -- it did NOT cause the incident measured on 2026-08-07, where
the server-side visibility log shows no create was ever built. It is committed
on its own evidence, not as a second explanation of that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HandleBossKilled computed a full reward -- doubled experience and money for the first run of the day, plus the satchel item -- built an LFGRewards from it, and then only ever announced it. Nothing granted anything. A grep for ModifyMoney, GiveXP or StoreNewItem across the LFG sources returned nothing at all, so finishing a dungeon paid exactly zero. The announcement did not arrive either, because SMSG_LFG_PLAYER_REWARD is not admitted through the enter-world send gate, so the whole feature was silent in both directions. Money and experience are now granted directly. GiveXP is a no-op at max level, which is the behaviour we want: a level-capped character keeps the money component and nothing else. The satchel goes through GiveDungeonRewardItem, which stores what fits and mails the remainder. Full bags are the expected case here, not an edge case -- the player has just looted a boss -- and a reward silently discarded because there was no free slot is worse than no reward at all. Mailing the overflow is what the achievement reward path in this server already does. RegisterPlayerDaily is now called as well. It had no callers whatsoever, so HasPlayerDoneDaily was permanently false and every single run took the first-of-the-day branch: doubled reward and a fresh satchel, indefinitely. It is recorded after the multiplier and item have been chosen, so the run that sets it still pays the first-run rate. The reward announcement itself still does not reach the client. Admitting SMSG_LFG_PLAYER_REWARD through the send gate is deliberately NOT done here -- its body has not been verified against the binary yet, and the gate exists because an unconverted body can crash the 18414 client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AttemptToKickPlayer and CastVote existed but had no callers whatsoever, and CMSG_LFG_BOOT_PLAYER_VOTE was declared and never registered. The whole feature was dead code: the client's Remove entry did nothing at all in a dungeon group. The vote body, derived from the 18414 writer rather than a fork: the packet class at vtable 0xD63364, header virtual sub_661F56 writing opcode 6078 (0x17BE), body writer sub_688B4B, which is exactly WriteBit(agree) + FlushBits. One byte, 0x80 agree / 0x00 deny, MSB-first. No guid, no length, no second field -- so the client says only HOW someone voted. WHICH vote it belongs to must come from server state, hence the session identifies the voter and the voter's group identifies the boot. Initiation is CMSG_GROUP_UNINVITE_GUID. In an LFD group a removal request becomes a vote instead, which is why the client collects `reason` there -- it has no other consumer. The branch sits deliberately BEFORE CanUninviteFromGroup, which demands leader or assistant: an LFD group has no meaningful leadership here, any member may start a vote including against the leader, and routing through the normal path would both refuse ordinary members and, for a leader, perform a real removal nobody voted on. Defects fixed in the pre-existing code, each of which was reachable: - CastVote dereferenced pPlayer->GetGroup() unchecked. The vote body carries no group, so any client can send one while ungrouped. Straight crash. - SendLfgBootUpdate dereferenced answers.find(guid) unchecked. The target is skipped when results are broadcast and a late joiner never had an entry, so end() was reachable. Straight crash. - The tally used `yay == REQUIRED_VOTES_FOR_BOOT`. Exact equality on a counter that is only tested after it reaches the threshold happens to hold today, but it silently does nothing if it is ever crossed by more than one. - m_bootStatusMap was never erased, despite the comment saying it was. The stale entry meant one vote per group, ever. - The booted player was never teleported out. They stayed inside the instance and could simply walk back, which makes the removal pointless. - A survivor of a FAILED vote was left in LFG_STATE_BOOT for the rest of the run, because the target was skipped in the state-restoring loop. That blocks every later vote in the group and is invisible until someone tries. - Nothing expired a vote. RemoveOldBoots now reaps at LFG_TIME_BOOT (30 s, measured across all 14 observed retail boot sessions) and always FAILS the vote: a kick needs explicit agrees, so silence must never remove anyone. Added guards, all reported with SMSG_PARTY_COMMAND_RESULT, whose flat 18414 body is already verified and admitted: a boot already in progress, a finished dungeon, and a group too small for the threshold to be reachable at all. That last one matters -- everyone except the target may vote yes, so a group of REQUIRED_VOTES_FOR_BOOT or fewer can never pass, and starting the vote anyway would freeze the group until expiry. Voting is also refused from the target themselves and from anyone who was not in the answer map when the vote started, so a member joining mid-vote cannot tip a tally they were never counted in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Devin SWE-1.7 Max reviewed 38bdd75..67937535b and returned BLOCK with two blocking findings and one important one. All three were verified against the source before fixing; all three were real. BLOCKING 1 -- partial reward loss. GiveDungeonRewardItem gated the store on CanStoreNewItem returning EQUIP_ERR_OK. On a PARTIAL fit _CanStoreItem fills `dest` with the portion that does fit (PlayerItemValidation.cpp:711-719 returns the error only after _CanStoreItem_InInventorySlots has already populated it), reports the remainder through no_space_count, and returns an error. So the storable portion was discarded and only the overflow was mailed: the player lost part of the reward. Now driven off `dest` and noSpaceCount rather than the return code. This is the ordinary case for a two-item reward with one free bag slot, not an exotic one. BLOCKING 2 -- the speed clamp was bypassable, and my justification for its placement was wrong. The previous commit claimed ObjectUpdate.cpp was "the only place the create block is filled in". It is not: Map::SendInitSelf:1891-1899 builds the player's own create with raw GetSpeed values. Both paths funnel through MopUpdateObject::AppendSimpleLivingMovement, and block types 1 and 2 both reach the same client validator via sub_79DC30, so the self create is validated identically. A player whose speed was clamped to zero would have had their OWN create rejected -- strictly worse than an observer create, since the player would not exist on their own client at all. Moved the clamp into AppendSimpleLivingMovement, where every create block is actually serialised, and removed it from the call site. One source of truth, and no future writer can bypass it by construction rather than by comment. IMPORTANT 3 -- Item::CreateItem clamps count to the item's max stack size and creates exactly one stack, so mailing an overflow larger than a stack silently dropped the excess. Now loops until the remainder is exhausted. Latent today, since rewards are 1-2. The reviewer's remaining uncertainty -- whether the client applies the same near-zero test to UPDATEFLAG_LIVING values updates as to creates -- is not resolved here. The binary evidence covers the create validator only. Moving the clamp into the shared movement writer does not cover the values path, which has its own serialiser. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both had writers that were called on every relevant path and then dropped by the enter-world send gate, so neither had ever reached a client. Admitting them first required knowing the bodies were right; they were not. SMSG_ROLE_CHOSEN 0x1A1F. The old body was flat -- uint64, uint8, uint32 -- and this opcode is not flat. Layout derived from the 18414 reader sub_6E921A (its handler at 0x985605 logs "ROLE_CHOSEN - GUID: %016llX, Accepted: %s, Roles Desired: %x"): nine mask bits whose SIXTH is `accepted` rather than a guid bit, then guid bytes 0,3,6, then the roles dword, then 5,1,4,2,7. Eight corpus packets across seven captures reconstruct byte for byte under it, including two with accepted = 0 that also carry roles = 0 -- which confirms our existing `roles > 0` test for the flag matches retail. Registered with DefS and admitted. SMSG_LFG_TELEPORT_DENIED 0x063B. The body is FOUR BITS -- WriteBits(reason & 0xF, 4) then FlushBits -- not a byte, which is why every captured body is one byte long. That also dissolves the reason admission was withheld. The old comment argued the captured 0x10 lay outside our enum, so our codes must be wrong. The size was right and the reading was wrong: bits are MSB-first, so reason 1 sits in the high nibble and lands as exactly 0x10, and the corpus also carries 0x90, which is reason 9. Both are ordinary codes. The enum is corrected to the derived values -- FALLING 7, PLAYER_DEAD 9, FATIGUE 12, INVALID_LOCATION 15 -- and constrained to 0-15, because only the low nibble is transmitted and a larger value would silently truncate into a different reason. IN_VEHICLE, CHARMING and IN_COMBAT share 5, which routes to ERR_CLIENT_LOCKED_OUT: vague, but visible. 6 and 13 are silent in the client and are never sent, since either would reproduce the exact behaviour this commit exists to fix -- a click that does nothing and explains nothing. IN_COMBAT deliberately drops its provisional 30. That value came from the PARTY error dispatcher, and a four-bit field cannot carry it at all: it would truncate to 14. A vague visible message beats a confidently wrong one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it it The inherited SMSG_LFG_PLAYER_REWARD body shared no field order with 18414 and carried a uint8 flag the client never reads. Harmless only because the opcode was neither registered nor admitted, so the wrong bytes were discarded before reaching anyone. With the reward now actually being granted, the announcement should arrive too. Field MEANINGS are binary-derived, from the consumer at 0x989771 and the Lua accessor sub_986CDD behind GetLFGCompletionRewardItem. The client's own log lines name them: "LFG_PLAYER_REWARD - Queued Slot: %u, Actual Slot: %u, Base Money: %d, Base XP: %d" and "Receiving Item %u, Display %u, Quantity: %u". Field ORDER is corpus-derived and is labelled as such rather than claimed as binary-derived. 4634 appears nowhere as a literal -- the 5.4.8 client dispatches SMSG by an internal message index, not the opcode value, so the push-imm trick that works for CMSG does not apply -- and 0x989771 is reached through a runtime-computed pointer with no xrefs, so the deserialiser could not be walked. Thirteen corpus payloads decode with zero leftover under this order. Layout: money, queued slot, xp, actual slot, then a bit block carrying a 20-bit reward count followed by one is-currency bit per reward, then 16 bytes each of id / unknown / display id / quantity. Two details that decide correctness rather than cosmetics: - DisplayInfoID must be the THIRD reward dword. sub_986CDD takes the icon from entry+4 when the entry is an item, so putting the always-zero unknown there would leave the reward frame with no icon. - The is-currency bit is written 0. The currency branch divides quantity by 100 for high-precision currencies, so mislabelling an item as currency would misreport the amount rather than merely pick the wrong icon. QueuedSlot falls back to the concrete dungeon when the run was not random. The client masks ActualSlot (& 0xFFFFF) to look up the row it names and textures the alert frame from, so the two legitimately differ for a random run. Registered with DefS and admitted through the send gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AssertBytes reported mismatches through assert(false), which expands to nothing
under NDEBUG. These tests build in Release, so every mismatch printed its
diagnostic and then exited 0: the harness said OK while the bytes disagreed,
which is the one thing a byte-exactness test exists to prevent.
It had been failing. Running it printed
raid_finder_proposal: byte 104 is 0x08, expected 0x09
raid_finder_proposal: byte 108 is 0x09, expected 0x08
mop_lfg_proposal_packets_test: OK
The writer was not at fault. Roles occupy 25 uint32s starting at offset 44, so
byte 104 is role slot 15 and byte 108 is slot 16. The captured expected[] bytes
put 0x09 at slot 15; the hand-transcribed INPUT array put it at index 16. The
two halves of the fixture disagreed with each other, and since expected[] is
real captured traffic and the input array is a transcription of the same
capture, the transcription was wrong. Moved 0x09 to index 15.
The harness now records failures in a flag and returns 1 from main, so a
mismatch fails the run in any configuration.
Verified by injecting a deliberate one-byte corruption (proposalId 11132 ->
11133), rebuilding, and confirming the test reports
raid_finder_proposal: FAIL byte 35 is 0x7D, expected 0x7C
mop_lfg_proposal_packets_test: FAILED exit=1
then reverting and confirming a clean OK / exit=0. Asserting that a repaired
guard works without demonstrating it would have repeated the original mistake.
Found by the re-review of 67937535b..HEAD, which reported it as MINOR on the
grounds that it predates this branch. The broken assert does predate it; a
byte-exactness test that cannot fail does not stay minor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proposal-test fix showed the pattern; a sweep found four more tests whose
entire assertion mechanism was bare assert(). These targets build in Release,
where NDEBUG makes assert a no-op, so all four passed unconditionally.
They were worse than unchecked. assert does not EVALUATE its argument under
NDEBUG, so
assert(MopLfgSetRolesPackets::ParseRequest(packet, request));
never called the parser at all. These tests executed no code under test. Two
independent proofs fell out of repairing them:
- mop_lfg_set_roles_packets_test referenced PLAYER_ROLE_TANK, _HEALER and
_DAMAGE with no declaration in scope. It could not compile, and nobody knew,
because the only uses were inside assert(). They are declared locally now;
including LFGMgr.h for them drags the whole game library into the target and
the link then fails on unrelated globals.
- All four then failed to LINK, needing the WorldDatabase / CharacterDatabase /
LoginDatabase / realmID stubs that nineteen other tests in this directory
already define. They had never needed them because they had never referenced
the code under test.
Converted to the CHECK/g_fail idiom the healthy tests here already use, rather
than introducing a shared header, so a failure is recorded and returned from
main in any configuration.
One of the four was genuinely failing:
header: byte 4 is 0x8C, expected 0x8D
The writer is correct. SMSG_LFG_PLAYER_INFO's header is 20 + 1 + 17 = 38 bits,
so byte 4 holds six header bits; FlushBits pads the last two with zeros, giving
0x8C. The capture's 0x8D has the next bit set because in the real 206-record
packet those two bits are not padding -- they are the first bits of the lock
array that follows. The test sliced five bytes off that capture and compared
them against a standalone flushed header, asserting that our padding should
equal someone else's data. Narrowed to the 38 bits that are actually the header.
Full suite: 111/111 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AttemptToKickPlayer seeded the player being voted on with LFG_ANSWER_DENY, and CastVote's tally counted it toward `nay`. With REQUIRED_VOTES_FOR_BOOT = 3 that let a five-man kick fail on only TWO genuine denies, because the victim supplied the third for free. It also inflated the voteCount the client displays by a vote nobody cast. Retail does not poll the player being voted on. They are now left out of the answer map entirely, which is also what CastVote's membership check keys off -- so the victim cannot vote on their own removal by any route -- and SendLfgBootUpdate already tolerates a missing entry since the end() fix. Reported by the opcode derivation pass as gap G9. Its companion, the missing guard against starting a second vote while one is running, was fixed with the rest of the vote-kick work; this half was not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The victim does not vote, so a group of N has N-1 voters and the initiator is already AGREE. At N = 4 that caps denies at 2, below REQUIRED_VOTES_FOR_BOOT, so such a vote can pass on votes but can only fail by running out LFG_TIME_BOOT. That is a correct outcome and RemoveOldBoots delivers it. The point of the comment is that the reaper is therefore load-bearing, not belt-and-braces: removing it would wedge a four-man group in LFG_STATE_BOOT permanently. Retail avoids the corner by scaling votesNeeded with group size -- the corpus shows 13 for a 25-man LFR -- which we do not do. Found while verifying the previous commit rather than by testing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nknown
The client sends this immediately after CMSG_BATTLE_PAY_GET_PURCHASE_LIST on
login -- capture-000234 seqs 28138 and 28139, adjacent -- and we declared only
the second of the pair, so every login logged
SESSION: received not handled opcode UNKNOWN (0x0DE0)
Value and shape are corpus-confirmed at build 18414: thirteen occurrences across
as many captures, every body zero bytes, no direction conflict.
Deliberately not answered. Retail replies with the whole store catalogue --
SMSG_BATTLE_PAY_GET_PRODUCT_LIST_RESPONSE, 3517 bytes in capture-000234 seq
28146 -- and we have no store to describe. Guessing an empty-catalogue layout
would put underived bytes on the wire for a feature that does not exist here.
Saying nothing leaves the store unavailable, which is true.
Registered anyway, because the unknown-opcode line is how a genuinely
unrecognised opcode gets noticed and a known one repeating in it hides the
signal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A dungeon finder group survives a restart; its instance does not. An ordinary
dungeon's persistent state expires two hours after creation, so a party left
assembled overnight comes back with groups.groupType still carrying
GROUPTYPE_LFD and its group_instance bind already cleaned away.
RestoreDungeonGroup cannot help, and correctly does not try: it rebuilds a run's
LFG status FROM the bind, and the loop that calls it iterates binds, so a group
with no bind is never visited. The group returns flagged as a finder run with no
run behind it.
That half-state is worse than either end of it. Group::SendUpdate sets
`isLfg = isLFGGroup() && GetGroupDungeonEntry(...) != 0`, which is now false, so
SMSG_GROUP_LIST carries no LFG block and the client zeroes its LFG fields -- no
eye, no teleport options, no Vote Kick gate. Meanwhile every server-side
isLFGGroup() test still answers yes, so the teleport path refuses with "group
has no LFG status" rather than moving anybody.
Observed live 2026-08-07 11:33: logged into Wailing Caverns still grouped, no
eye, and
ERROR:LFG TeleportPlayer: Humanwarrior refused (out) --
group Group (Guid: 1) has no LFG status
Only the portrait's Leave Instance Group entry got the player out. The bind was
correctly absent -- the previous night's instance was created at 00:10 with a
13:33-style two-hour resettime and had long expired -- and "Loaded 0
group-instance binds total" confirms nothing was lost.
So the fix is not to restore more, it is to stop claiming the run exists. After
the binds are loaded, any group still flagged LFD whose dungeon entry resolves
to 0 is converted to an ordinary party and the change is persisted, using the
same predicate SendUpdate uses so a group is demoted precisely when the client
would otherwise have been left in the half-state.
ClearLfgGroup is the counterpart to SetAsLfgGroup and writes groupType through
to the database for the same reason that one does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings from the third pass, which returned APPROVE with one MINOR. MINOR: ClearLfgGroup carried "Converts the group to raid mode and refreshes related state." That brief was already misplaced -- it sat above SetAsLfgGroup, which is not ConvertToRaid either -- and inserting ClearLfgGroup between the block and its function moved the wrong text onto the new function. Both now say what their function does. The reviewer also sharpened the vote-stall analysis, and the comment is the whole deliverable of 52356f6 so it has to be right. I had recorded only the N = 4 case. N = 5 stalls too: with the victim excluded and the initiator already AGREE, a 1-agree/2-deny split among the three remaining voters leaves both counts at 2, below the threshold, so it also resolves only by expiry. Same conclusion -- the reaper is load-bearing -- but it applies to the ordinary five-man case, not just a depleted party, which makes it considerably more than a corner note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Investigating a loot window that opened and closed after about a second. The
symptom turned out not to be a bug -- retail does exactly the same thing, and
the corpus proves it: capture-000004 seq 17370 and capture-000009 seq 28146 both
show CMSG_LOOT_UNIT, SMSG_LOOT_RESPONSE, then CMSG_LOOT_RELEASE roughly a second
later. "Response then release" is the retail signature, not a fault.
The investigation did find a real defect underneath it.
LootSlotType is inherited and predates this build, and we cast it straight onto
the wire. A sweep of 9437 SMSG_LOOT_RESPONSE packets at 18414 finds the 3-bit
slot field takes exactly {3, 4, 7} -- 4 on 1809 ordinary drops, 3 on 542, 7 on
34 -- and 0, 1, 2, 5 and 6 never appear. Our LOOT_SLOT_NORMAL is 0.
The client branches on it, at loot-record offset +24:
- the auto-loot pass (sub_9387D6, 0x938824) takes a slot ONLY when it is 3 or
4, so our 0 was silently skipped and never auto-looted;
- only 4 suppresses the bind-on-pickup confirmation (0x937DCB), so our 0
raised a BoP prompt where retail shows none;
- 2 opens the master looter list, 5 reports locked, 7 refuses the click.
It only misfires in a GROUP. A solo kill resolves OWNER_PERMISSION to
LOOT_SLOT_OWNER, which is already 4 and correct; shared loot resolves
LOOT_SLOT_NORMAL, which is 0 and is not. The live repro was in a group -- header
byte 0 was 0x20, i.e. hasLootMethod set.
ToWireLootSlotType now maps the internal enum onto the client's space: OWNER 4,
NORMAL 3, MASTER 2, VIEW and REQS 7, and never 0, 1, 5 or 6. 2 is reachable only
from LOOT_SLOT_MASTER; it has no corpus support, but a master-loot row has to
say so somehow and every other value would misrepresent it.
Two byte-fidelity fixes alongside:
- The per-item 2-bit field is 3 on all 67 corpus item records and we sent 0. The
client parses it into msgItem+24 and never reads it back -- sub_9D5F3D, the
only consumer of the item array, touches +0,+4,+8,+12,+16,+28,+32,+36 -- so
this changes no behaviour. It was my first suspect for the close and it is
provably not.
- The trailing optional byte is now always emitted, value 17. Despite the field
name it is not a failure reason: the client reads it into msg+40 and consults
it only on the success == 0 branch. Retail ships it on all 32 decoded
successes and omits it in only 3 of 9437 packets, all failures.
Deliberately NOT changed: lootGuid. Retail's loot handle is HIGHGUID 0xF190 with
a zero entry field, and we send the creature GUID, which is structurally
impossible in retail traffic. That is a real lead, but the same value is the key
our own CMSG_LOOT_RELEASE and autostore matching uses (LootHandler.cpp:84), so
changing it is cross-cutting and needs its own investigation and a live retest.
The 1-second close is still unexplained, and may well be normal auto-loot
behaviour. The cheap discriminator is to repeat the pull SOLO, where slotType
was already 4: if the window still closes, slot type was never involved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of fcdee00 returned BLOCK with two findings. Both were right. BLOCKING: currency rows still put the raw internal LootSlotType on the wire. LootMgr.cpp:1289 assigned personalSlotType directly, so for group loot it sent LOOT_SLOT_NORMAL = 0 -- exactly the value the previous commit exists to stop sending. Currency was skipped by the auto-loot pass and raised a bind confirmation, for the same reason items did. Now goes through ToWireLootSlotType like the item and group-roll paths. This one was a plain miss rather than a judgement call: the grep that showed me the item assignments listed this line too, and I converted the ones above it and walked past this one. A sweep for raw assignments now returns nothing. IMPORTANT: the default arm returned 3, which is takeable and auto-loot eligible. The switch already covers every valid slot type 0-4, so the default is reachable only from MAX_LOOT_SLOT_TYPE -- the sentinel meaning "skip this row" -- or from a corrupt value. Offering such a row to the player and then rejecting the click server-side is the worse failure mode, so it returns 7 and fails closed. Full suite still 111/111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whole-branch review finding. The range reviews could not see it: the commit that made it reachable and the code that leaks are sixty commits apart. m_playerStatusMap is keyed by player guid and outlives the group. Nothing on the leave or disband path ever cleared it, which was harmless while no player state survived long enough to matter. Vote kick changed that. AttemptToKickPlayer sets every member to LFG_STATE_BOOT, and if the group then disbands -- members leaving one at a time, or the last one quitting -- ReleaseGroupLfgStatus erased the two group maps and left every player state behind. They persisted across relog, because the map is cleared on neither login nor logout, and HandleLfgGetStatusOpcode ships whatever it finds. The client was handed LFG_STATE_BOOT for a vote that no longer existed, in a group the player had left, and could raise a boot dialog nothing would ever resolve. It cleared only if the player happened to re-queue, or the world restarted. ReleaseGroupLfgStatus now takes the Group rather than its guid, resets every member slot to LFG_STATE_NONE, and erases any boot entry. Deferring the boot entry to the reaper is not enough: the reaper resolves the group to restore state, and by then there is no group. RemoveOldBoots gains the matching else-branch. When the group is gone it clears the polled players and the victim outright, since there is no dungeon state left to restore them to. Also corrected three comments that my own later commit made false. They said SMSG_LFG_TELEPORT_DENIED was not admitted and refusals were therefore silent; 569cf84 derived its four-bit body and admitted it. The code was right and the comments were describing the world before it. Full suite 111/111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deep whole-branch audit findings. Three fixes; the first is the real one. IMPORTANT -- a merged solo queuer was silently destroyed. LeaveLFG's group branch removed every party member through RemovePlayerFromQueue and then erased m_queueSet[grpGuid] and m_playerData[grpGuid] unconditionally. RemovePlayerFromQueue only erases the entry once currentRoles empties, which is correct, so the extra erase was redundant in the ordinary case -- and destructive in the case that matters. MergeGroups can absorb a solo queuer into a party's entry, and that player is still in currentRoles after the whole party has left. The erase took their queue with the party's. Their m_playerStatusMap still read LFG_STATE_QUEUED while they were gone from m_queueSet and m_playerData, so no match and no queue-status update could reach them again: they sat believing they were queued until they manually left and re-queued. Reachable whenever a party of fewer than five absorbs a solo queuer and then cancels. Now only torn down when nothing is left in it. MINOR -- CastVote tested `RemoveMember(...) <= 1` for disband. RemoveMember returns the surviving count, and an LFG group is now allowed to live on with one member, so 1 no longer means disbanded and the branch would delete a group still in play. Unreachable today, because REQUIRED_VOTES_FOR_BOOT = 3 stops a vote starting below five members, but it is a use-after-free the moment that constant is lowered -- which the LFR work will want to do. Tests for zero now. MINOR -- a player who LEAVES an LFG group mid-vote kept their state. The previous commit cleared members when the group disbands, and the reaper clears the polled players when the group is gone; someone who simply walks out is in neither set. They kept LFG_STATE_BOOT across relog and HandleLfgGetStatusOpcode would hand their client a boot dialog for a vote that no longer existed. OnPlayerLeftLfgGroup now always runs on that path, clears their state and withdraws their vote -- an absent player should not count toward a threshold they can no longer be persuaded to change. It is deliberately separate from OnPlayerLeftDungeonGroup, whose early returns are right for deciding Deserter and wrong for cleanup. Full suite 111/111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…FG groups Sliced deep audit of the group-lifecycle files, which no previous reviewer had looked at. Two BLOCKING findings, same root cause seen from opposite ends: this branch let LFG groups survive with one member, and two callers that read RemoveMember's return value to decide whether it disbanded were never updated. BLOCKING -- use-after-free on the most ordinary leave path there is. Player::RemoveFromGroup tested `RemoveMember(...) <= 1` and freed the group, commented "already disbanded in RemoveMember". RemoveMember returns the SURVIVING count, and its threshold is now `GetMembersCount() > (isBGGroup() || isLFGGroup() ? 1 : 2)`, so removing one of two members takes the ordinary removal branch, returns 1, and never disbands. The old test then deleted a live group: the survivor kept a dangling m_group (only Disband nulls it), the groups and group_instance rows were left behind (only Disband deletes them), and the LFG status leaked (ReleaseGroupLfgStatus is called from Disband). The next GetGroup() reads freed memory. Reachable by a dungeon run bleeding to two players and one clicking Leave Instance Group. Also from proposal creation dissolving a finished two-member group, from character deletion, and from Eluna's player:RemoveFromGroup(). I fixed this exact defect in LFGMgr::CastVote earlier and did not check for other callers. This is the high-traffic one. BLOCKING -- the startup cleanup loop disbanded the groups the branch exists to preserve. ObjectMgrInstanceData.cpp dropped every group with fewer than two members, which was correct while logout dissolved one-member groups. It no longer does: an LFG group survives logout deliberately, so a run down to a single member persists and loads. The loop then disbanded it BEFORE the instance-bind loop, so RestoreDungeonGroup never ran for it, and before the demotion sweep, which could no longer see it. The player logged back in inside the instance with no group, no LFG block, no eye and no teleport out -- precisely the stranding this work exists to prevent, undone by a pre-existing loop nobody updated. The threshold now mirrors RemoveMember's. MINOR -- refusing to leave in combat reported ERR_PARTY_RESULT_OK, the same code as success, so the player got an "OK" and stayed in the group with nothing to explain it. Now ERR_PARTY_LFG_TELEPORT_IN_COMBAT, which is the client's own message for this case. The comment above it, which I made inaccurate in an earlier commit, is corrected too. Full suite 111/111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed the 1e-6 floor sat an order of magnitude clear of the client validator bound. It does not: 1e-6 over 2^-22 is about 4.2x. The floor is still ample, since the comparison is exclusive and nothing representable below it reaches the wire, but the overstatement is the dangerous kind -- a maintainer trusting ten times the headroom could lower the floor toward 3e-7 and land inside the rejection band. Found by a sliced audit of the object-update files, which no earlier reviewer had read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sliced audit of the test files. Both helpers detect a size mismatch, record it through CHECK -- which does not abort -- and then compare bytes anyway, indexing by the EXPECTED length rather than the actual one. A short packet is therefore read past its end. The vector's spare capacity usually absorbs that, which is worse than a crash: the run prints a wall of byte mismatches for bytes that do not exist, so a size bug presents as a content bug at precisely the moment someone is trying to diagnose it. That is the opposite of what these tests are for. Neither can fire while the writers are correct, which is why a passing suite never showed it. They fire on the first writer regression, which is the run whose output most needs to be trustworthy. mop_lfg_role_check_packets_test now returns after reporting the size mismatch. mop_lfg_player_info_packets_test gates its byte comparison on the size being right, since its AssertBytes takes a raw pointer and bounds-checks nothing. Full suite 111/111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MadMaxMangos
force-pushed
the
fix/lfg-matchmaker-role-resolution
branch
from
August 7, 2026 22:24
38bdd75 to
6ccf5bf
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
MadMaxMangos
changed the title
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
1 participant
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.
Makes the dungeon finder work end to end, then keeps going until the surrounding systems it touches work too. It was broken at every link, and any one of them was fatal on its own.
Confirmed working against five real 5.4.8 clients: queue as a group, role check, proposal, all five entering the instance in the same tick with no phasing, teleport out and back, loot arriving, and leaving cleanly.
The original breakage
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 client DifficultyID against the internal 0-based enum. No queueable row inLfgDungeons.dbccarries DifficultyID 0, so the branch never fired, every entry reported needing nobody, andRoleMapsAreCompatiblethen 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 consumer switched on the exact values0x02/0x04/0x08, so a hybrid counted as zero of everything.What the branch adds beyond the original fix
A live run, not just a formed group. Teleport in and out, entry points captured once at queue time so walking out returns you where you queued rather than the dungeon doorstep, dungeon completion rewards actually granted (money, XP, satchel with mail overflow), the requeue cooldown and Dungeon Deserter, and vote kick implemented end to end.
Survival across a restart.
GROUPTYPE_LFDnow persists; a run still in progress is rebuilt from its instance bind at startup; a group whose instance expired is demoted to an ordinary party instead of being left half-LFG; and LFD groups survive down to a single member rather than dissolving under the player.Wire fidelity. Every packet body here is derived from the 18414 client binary or decoded from real capture bytes, never from a reference fork. Several were rebuilt from pre-MoP shapes that only ever worked because the enter-world send gate was discarding them:
SMSG_ROLE_CHOSEN,SMSG_LFG_TELEPORT_DENIED(a four-bit reason field, not a byte),SMSG_LFG_PLAYER_REWARD, andSMSG_WHO.Fixes outside LFG that the work uncovered
These were found while chasing LFG symptoms and are genuine defects in their own right.
Players invisible to each other after a group teleport.
m_clientGUIDs— the server's record of what each client holds — was never cleared on a far teleport, but the client destroys its entire object manager onSMSG_NEW_WORLD. Upstream relies on a leftover sweep to catch this, which works only if an observer's visibility pass runs between one player leaving and the next arriving. Teleporting a whole party into a dungeon removes that gap, so the first player out was never erased from anyone's set,HaveAtClient()stayed true, and no create was ever sent again. Measured: five players entering Wailing Caverns in the same second, a strictly triangular erase pattern, and the one player who entered first missing for all four others.CMSG_OBJECT_UPDATE_FAILEDis now handled. The client had been telling us which object it could not build and we were discarding the message. An existing handler was never registered — and could not have been, since its GUID layout was another build's. Now decoded with the layout the 18414 writer actually emits, and used to repair the bookkeeping.Loot slot types. The inherited
LootSlotTypeenum was cast raw onto the wire. A sweep of 9437 retail packets shows the 3-bit field only ever takes{3, 4, 7}— never0, which is what we sent for group loot. The client auto-loots only slots 3 or 4, so items were silently skipped and a bind-on-pickup prompt appeared where retail shows none.Movement speeds that made the client discard a packet. The create validator rejects an object whose speed is approximately zero, and a rejected create makes the client drop the rest of the packet. Shaman totems legitimately have
SpeedWalk 0, andUnit::UpdateSpeedmultiplies straight through it. Clamped at the emitter, so the unit keeps its real rate and only the wire value is floored./whoimplemented for 18414, both request and response, verified by decoding retail's own reply with the same reader.Review
Ten independent review passes: four ranged reviews (SWE-1.7), one whole-branch pass, and five per-subsystem deep audits (GLM-5.2) covering all 37 changed files. The whole-branch pass alone covered only 8 of 37, which is why the slices exist.
Findings that mattered, all fixed:
Player::RemoveFromGroup— it testedRemoveMember(...) <= 1for "already disbanded", which stopped being true once LFG groups were allowed to live with one member, so an ordinary two-person leave freed a group still in playLFG_STATE_BOOTleaking into player state across relog, showing a phantom boot dialogassert()is a no-op underNDEBUG, and since it does not evaluate its argument they never called the code under test at allTesting
ctest -C Release— 111/111.Live, against five clients: group queue, role check, proposal acceptance, five simultaneous instance entries with clean party frames, teleport out and back, group loot arriving, group leave with no crash and no orphaned rows, and the startup demotion sweep firing correctly.
Known and deliberately deferred
SMSG_LFG_DISABLED/SMSG_LFG_SLOT_INVALIDunimplemented — zero corpus rows and an unconfirmed opcode value. Registering a guessed value is worse than leaving them dormant.LFGRewardscarries a single item and always writes is-currency 0; retail can send multiple rewards and currency. Needs the reward data model extended.REQUIRED_VOTES_FOR_BOOTis hardcoded to 3; the corpus shows 13 for a 25-man LFR, which is not wired.lootGuidcarries the creature GUID where retail uses a0xF190handle. It keys our ownCMSG_LOOT_RELEASEand autostore matching, so changing it is cross-cutting and wants its own change.dungeonfinder_rewardscovers levels 15-80, so MoP-level characters are paid nothing. Data gap.SMSG_LFG_BOOT_PLAYERthedidVoteandagreebits are not distinguished by any captured packet — every sample has both set — so that assignment rests on the client's struct field order. A live vote of "no" settles it, and it is a one-line flip if wrong.