LFG: fix silent packet drops in the transfer window, torn queue identity, and the group queue burst - #83
Merged
Merged
Conversation
Three findings from the sliced deep audits of this branch. JoinLFG tore the player out of their existing queue at the top of the function, but validation of the new selection happens ~240 lines later. A re-queue that is then refused -- selecting random category 434, whose own row is the only member of Group_ID 33, so the candidate set filters to empty and the no-slots gate rejects -- left the player queued for nothing, with no indication they had lost their place. Defer the removal until after every join gate has passed. Nothing between the two points re-queries queue state, and the rejection gate is the only return in that span, so the old entry is now kept on every refusal path. The comment claiming the gate "refuses the join before a queue entry exists" was true only for a first-time queue; corrected. Group::ResetInstances always dropped the bind for a difficulty change, including when DungeonMap::Reset returned false because players were inside. Since this branch admits SMSG_INSTANCE_RESET_FAILED through the send gate, the leader was told PLAYERS_INSIDE while the bind was destroyed anyway. isEmpty is exactly Reset()'s return, so removing the CHANGE_DIFFICULTY special case makes the message and the outcome agree in both directions. Pre-existing; this branch only made it visible. m_resetAfterUnload deletes respawn times only, not encounter state, so the retained bind keeps its real progression. A random category with no runnable member leaves the entry queued and retries every tick, which is correct, but fired sLog.outError each time. Demoted to DEBUG_LOG. Fixes authored by Codex against an audit brief; body layout of the instance-reset family separately confirmed in IDA (consumer at 0xCE0CB2 reads reason at +0x10, map id at +0x14) and left untouched. Build clean, 119/119 ctest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two players backfilled into an LFG group already inside a dungeon saw every pre-existing member as "Unknown" in the party frames, while those members saw the joiners' names correctly. The joiners were between teleport and worldport ACK, so _player was set but IsInWorld() was false. CMSG_NAME_QUERY was registered STATUS_LOGGEDIN, and that dispatcher case executes only under IsInWorld() with no else branch -- the packet is discarded with no log line and no response. The client never re-asks, so the name stays "Unknown" for the rest of the session. The comment directly beneath that case already noted transfers could arrive there. Confirmed against the packet log: 29 CMSG_NAME_QUERY received but only 23 SMSG_NAME_QUERY_RESPONSE sent. The six unanswered decode (XOR-1) to the guids of the three members already inside; the answered ones are the two joiners, queried by sessions that were in world. Neither the request parse, the guid, the DB lookup nor the send gate was involved -- SMSG_NAME_QUERY_RESPONSE was already admitted by IsEnterWorldConverted. STATUS_LOGGEDIN_OR_TRANSFER runs the handler whenever _player exists. HandleNameQueryOpcode only reads a guid and sends a name, so it needs no world state. Root cause found by Codex against a live-evidence brief; dispatcher semantics and the packet-log correlation verified independently. Build clean, 119/119 ctest from PowerShell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three files added by this branch carried an abbreviated SPDX-only header. Every other source file in the tree, including the pre-existing tests in the same directory, uses the full 24-line block with the project description, copyright range and Blizzard notice. Match it, and keep each file's own one-line description beneath. No functional change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 27efb7a, from the review of that fix. CMSG_NAME_QUERY was not the only opcode being silently dropped in the transfer window: the STATUS_LOGGEDIN dispatch case runs the handler only under IsInWorld() and has no else branch, so anything the client sends between teleport and worldport ACK is discarded with no response and no log line. Four more carry handlers that touch no player or map state, so they are safe to run mid-transfer, and all four response opcodes are already admitted by IsEnterWorldConverted: CMSG_CREATURE_QUERY GetCreatureTemplate, static read CMSG_GAMEOBJECT_QUERY GetGameObjectInfo, static read CMSG_QUERY_TIME server time and daily reset countdown CMSG_REALM_NAME_QUERY cached realm name The first two are the ones that show: on a cold WDB cache the client queries every creature and gameobject it sees as the destination map loads, which is exactly the transfer window, and a dropped query is not retried until after ACK -- visible as name and model pop-in. Deliberately NOT changed, because their handlers need a valid map or mutate player state: CMSG_PET_NAME_QUERY (GetAnyTypeCreature), CMSG_NPC_TEXT_QUERY (SetTargetGuid), CMSG_INSPECT (IsWithinDistInMap) and CMSG_QUEST_QUERY (PlayerTalkClass). Each handler was read to confirm it never touches _player, GetMap, GetPlayer or IsInWorld. Build clean, 119/119 ctest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JoinLFG resolved the existing queue entry as `pGroup ? guid : FindQueueEntryContaining(guid)`, so a party looked up only its own group key. That key is frequently not where the party lives. MergeGroups merges into its first argument (LFGMgr.cpp:1468), and the caller passes the outer iteration value of a std::set<ObjectGuid> snapshot, so the lower raw guid absorbs the other. HIGHGUID_PLAYER is 0x000 while HIGHGUID_GROUP is 0x1F5 shifted left 48/52 bits, and operator< compares raw values -- player guids always sort first. When a solo queuer and a partial party match, the SOLO entry absorbs the party and the group's own key is erased while its members stay listed in currentRoles. The party's next queue therefore resolved nothing, skipped the replacement path entirely, and built a second live entry for players the solo-keyed entry still listed. Both could then produce competing proposals for the same people. Resolve the entry by the player's guid in both cases, which finds it whether it is group-keyed or solo-keyed, and end that whole entry when the replacement passes its gates: snapshot the listed members, send each a terminal LFG_UPDATE_LEAVE while their retained ticket is still active -- the client keys status records by RideTicket, so clearing server state alone leaves an orphaned minimap eye -- clear each status, then erase the resolved key. The earlier RemovePlayerFromQueue loop is gone; it recomputed needed roles after every removal on an entry being destroyed anyway. Root cause identified and fixed by Codex after it blocked an earlier attempt of mine that cleaned the group key -- the case that does not arise from a merge. Guid ordering, merge direction and the caller were verified independently before accepting. Not verified live: the absorbed solo clearing their eye, and no later proposal from the stale entry. Build clean, 119/119 ctest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 0ea7d6f, closing the reservation an Opus review raised against it. The client files every status body under the whole 20-byte RideTicket -- requesterGuid, ticketId, ticketTime, ticketType. WorldSession::SendLfgUpdate derived requesterGuid from the player's CURRENT group via an isGroup flag, so any player whose grouping changed while queued got a terminal body keyed to an owner the client had never seen: it opened a second record and left the first lit at joined=1, which is the stuck eye this branch exists to kill. Nothing clears queue state on group join, so it is reachable both ways -- announced solo then joins a party, announced group then leaves it. CancelProposal is not a counter-example. proposal.groups does hold the original group guid, but it was reduced to a bool and SendLfgUpdate rebuilt the requester from the current group regardless, so a player who moved from group G to H still got H there too. RetainedTicket already exists to survive merges and entry erasure, which LFGPlayers does not, so the requester now lives there as part of one indivisible identity: recorded when the queue is created, first-wins through MergeGroups, replaced atomically by BeginTicket only for a genuinely new queue, and read directly when a body is built. isGroup survives as fallbackIsGroup for paths with no retained identity, so nothing regresses. Also prefers a party's own live entry when resolving the existing queue, falling back to the entry that lists the player only when a merge erased that key -- the reviewer's minor. Wait-time counts are deliberately untouched: playerCount is incremented and never read, and making it operational needs an accounting audit across every leave, merge, proposal, timeout and success path, not a local edit here. Authored by Codex, which proposed this shape after reviewing its own fix. Build clean, 119/119 ctest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the IMPORTANT an Opus review raised against 5f3d3c6: that commit moved SMSG_LFG_UPDATE_STATUS onto the retained requester but left its sibling SMSG_LFG_QUEUE_STATUS deriving one from current grouping, so the two packets -- filed by the client under the same RideTicket -- could disagree. The comment there still claimed it mirrored SendLfgUpdate, and after that commit it no longer did. Reachable without a merge: group G {L,P} queues, so P's identity is retained as (G, id, t) and the opening body is filed under it. P leaves G; nothing clears queue state, so P stays QUEUED in G's currentRoles. Every tick SendQueueStatusFor then sent P a status keyed under P, which the client -- tracking (G, id, t) -- discards: no role counts, no average wait, and most of the eye's tooltip missing for the rest of the queue. Not a stuck eye, since the update bodies still reach and clear the record, but a persistent cosmetic failure on the mainline path. This body's key is queueGuid + clientQueueId + joinTime, so all three now come from the retained record rather than the live entry. Requester alone was not enough: - ticketId, because MergeGroups can leave the surviving entry carrying a different id than the one the client was told. - joinTime, which bites with no merge at all -- PerformRoleCheck overwrites the entry's joinedTime when the check completes, so a group whose role check took a few seconds was told a start time its client never recorded. Found by a Codex review of the first version of this commit, which fixed only requester and id. timeSpentInQueue is measured from the same retained start, so the body no longer states one join time and a duration counted from another. The old group derivation stays as the fallback for a player with no retained identity. Corrects the RetainedTicket doc-comment as well: it claimed ForgetTicket clears the record, but ForgetTicket has no callers. BeginTicket's wholesale replacement on the next join is what actually keeps it fresh. Build clean, 119/119 ctest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in SMSG_LFG_JOIN_RESULT, the third packet of the identity
triple the code already names ("One identifier, three packets",
LFGHandler.cpp:617).
First, the success body hardcoded requesterGuid to the player's own guid
and looked its ticket up player-keyed, so a group queue announced itself
under an identity differing from the update and queue-status bodies, which
now carry the retained group requester. Retail disagrees, on wire evidence:
capture-000075 seq 891753 under catalogue 2BE10C89... carries
f0 00 00 14 00 00 48 06 8f d2 53 92 46 00 00 03 00 00 00 55 b5 f1 1e 13
whose requester decodes to 0x1F5400001249B4F0 -- HIGHGUID_GROUP 0x1F5. Its
six non-zero raw bytes F0 B4 49 12 54 1F appear XOR-1 obfuscated as
F1 B5 48 13 55 1E, and the two zero bytes are correctly absent from the
mask. The same body carries joinTime 0x53D28F06, clientQueueId 0x4692 and
ticketType 3.
All three fields now resolve from the retained record as one unit, with
the old player-derived values kept as the fallback for a queue with no
retained identity. Refusal bodies are untouched: retail zeroes the guid
and the whole ticket, which is what makes the 18-byte form, and all 11
observed refusals are that shape.
Second, found while tracing the first: the group path never sent a
successful join result at all. Only the solo sequence did, so a party
leader got status bodies for a queue the client was never told had opened.
It is now sent after the role check completes and before the group enters
the queue, matching the observed retail order. BeginTicket has already run
by then, so it quotes the same identity as every other body.
Corpus decode independently reproduced byte-for-byte before accepting;
the missing-send claim verified against the pre-change tree.
Build clean, 119/119 ctest. Live retest should confirm a leader receives
all three packets under one RideTicket and no second queue record appears.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The group path announced its queue out of order: reason 24 went out at join time, before the role check had even begun, and the completion burst was a single reason-13 body with no repeat. Retail, capture-000075 under catalogue 2BE10C89..., sends all four at completion: 891751 UPDATE_STATUS reason 24, joined true, queued false 891752 UPDATE_STATUS reason 13, joined true, queued true 891753 JOIN_RESULT 891754 UPDATE_STATUS byte-identical repeat of 891752 891752 and 891754 are byte-identical over all 40 bytes, and every body in the burst carries the same RideTicket as the join result -- requester f1 b5 (group-typed, XOR-1), ticket id 0x4692, time 0x53D28F06, type 3. Moving reason 24 off the join path leaves no silent window: the role check is covered by its own UI packets, 891708 ROLE_CHECK_UPDATE, 891749 ROLE_CHOSEN and 891750 the final ROLE_CHECK_UPDATE, with no UPDATE_STATUS before 891751 at all. The status bodies stay party-wide while the join result stays leader-only, now on direct evidence rather than inference. A non-leader's stream in capture-000499 carries 898014, 898015 and 898016 as three CONSECUTIVE UPDATE_STATUS bodies with no join result between them, against the leader's stream where it sits between the second and third. This settles a disagreement between two reviewers, one of which held the question unresolvable from a single-client capture. currentRoles and the queue state are committed before the burst is built, so all three bodies describe the accepted roles and nothing affecting construction changes between the two reason-13 sends -- which is what makes the repeat byte-identical rather than merely similar. Corpus decodes reproduced independently before accepting; opcode 3118 is 0x0C2E, our binary-derived SMSG_LFG_UPDATE_STATUS, not the QUEUE_STATUS the shipped name map labels it. Build clean, 119/119 ctest. Live retest: each member should see 24/q0 -> 13/q1 -> 13/q1 with only the leader receiving JOIN_RESULT between the two 13s, both 13 payloads identical per recipient, and no part of the burst on a failed or timed-out role check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….010
The server does not hard-require the companion database branch: that
migration bumps CONTENT and leaves STRUCTURE at 3, and only a structure
mismatch refuses startup. Without the table the entrance loader logs a DB
error and loads zero rows while physical entrances keep working, so an
operator running the server without the database gets a quietly degraded
dungeon finder and no clear signal. The content warning exists for exactly
this, so declare the dependency: WORLD_DB_CONTENT_NR moves from 1 to 10.
That alone would have been a flag that never fires. DBVersion stores every
field as std::string and the content test was
current_db_content < minimal_expected_content
which is lexicographic. With 10 expected and a database at 9, "9" < "10"
is false because '9' > '1', so no warning is emitted for a database that
is genuinely behind. The gate has been silently inert since content passed
9, and would have stayed inert for every future bump.
Content comparison now goes through an overflow-free decimal compare used
by the World, Realmd and Character checks alike:
db content 9 vs expected 10 -> Older, warning fires (was: silent)
db content 10 vs expected 10 -> Equal, no warning
db content 11 vs expected 10 -> Newer, no warning
Malformed or empty content is classified invalid and logged with both
values rather than silently comparing as some ordering; startup still
proceeds, since content mismatches remain advisory. Leading zeroes are
normalised and arbitrary-length values cannot overflow.
Version and structure keep exact string equality -- they are compared with
!=, where lexicographic equality is correct -- and a content mismatch
stays non-fatal. GenRevision.cmake was left alone; it fails a top-level
configure at :156 for unrelated reasons, but the generated
revision_data.h picks the new constant up, verified in the build tree.
Found by the user asking whether the server should declare its database
dependency -- three review rounds over this branch had not raised it.
Build clean, 120/120 ctest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Follow-up to the merged PR #82. Everything here came out of live testing against five 5.4.8 clients, and each wire claim is either corpus-decoded or confirmed on a live client.
Requires mangosfour/Database#4. Without it the entrance table is empty and the LFG-only path silently does nothing; physical entrances still work, so it degrades rather than breaks. The final commit declares that dependency so an operator gets a warning instead of a mystery.
The main thread
A live report — two players backfilled into a group see everyone as "Unknown" — unwound into four connected defects.
Packets silently dropped mid-transfer. The
STATUS_LOGGEDINdispatch case runs a handler only underIsInWorld()and has no else branch, so anything a client sends between teleport and worldport ACK is discarded with no response and no log line. The client never re-asks. Packet log: 29CMSG_NAME_QUERYin, 23 out; the six unanswered decoded to the three members already inside. Fixed for name query, then for four more transfer-safe query opcodes —CREATURE,GAMEOBJECT,QUERY_TIME,REALM_NAME— each verified to touch no player or map state, with all four responses confirmed admitted by the send gate. Four others (PET_NAME_QUERY,NPC_TEXT_QUERY,INSPECT,QUEST_QUERY) were deliberately left alone; their handlers need a valid map or mutate player state.Torn queue identity. The client files every LFG status record under the whole 20-byte RideTicket — requester + ticket id + time. The requester was derived from the player's current group, so anyone who regrouped while queued got bodies keyed to an owner the client had never seen: a second record, and the first left lit. The authoritative identity now lives on
RetainedTicket, which already survives merges and entry erasure, first-wins throughMergeGroupsand replaced atomically only by a genuinely new queue. Applied consistently across all three packets that share that key.A queue entry that outlived its party.
JoinLFGresolved an existing entry by the group's own key, butMergeGroupsmerges into the lower raw guid and player guids sort before group guids — so a solo queuer absorbs a party and erases the group key. The party's next queue then built a second live entry for players the old one still listed, and both could produce competing proposals.A packet never sent at all. The group path never sent a successful
SMSG_LFG_JOIN_RESULT; only the solo sequence did. Party leaders got status bodies for a queue their client was never told had opened.Retail wire order
The group completion burst now matches capture-000075 exactly:
Leader-only is proven, not assumed: a non-leader's stream in capture-000499 carries three consecutive status bodies with no join result between them. Two reviewers disagreed on this and the member capture settled it.
Confirmed live, decoded from our own emission: reason bytes
18/0D/0D, queued bit clear then set then set, bodies #2 and #3 byte-identical, and the same RideTicket (ticketId 0x03E9,joinTime 0x6A787BEE, type 3) across all four leader packets.Also fixed
0xCE0CB2: reason at+0x10, map id at+0x14; 0/2/3 map to strings, 1 is silent).Verification
Build clean, 120/120 ctest. Live: 82 minutes with five clients through queue, role check, merge, proposal, simultaneous entry, vote kick and removal, exiting cleanly with no faults. Door routing verified 11/11. Name resolution went from 6 dropped queries to 0, and on a cold WDB cache creature/gameobject queries answered 217/217 and 210/210.
Reviewed across GLM-5.2, Codex and Opus. They consistently found different defects — one caught a bug both others missed, and another was itself corrected on a point it had asserted — so findings were unioned, and every one verified against the source before acting.
Known and deliberately deferred
didVote/agreebit assignment is still unsettled — every captured retail packet has both set, so it needs one NO vote; we now know the exact two bit positions.ForgetTickethas no callers (bounded, one record per player, replaced on next join). Wait-timeplayerCountis incremented and never read; making it operational needs an accounting audit across every exit path, not a local edit.🤖 Generated with Claude Code