Fixed a bug that caused the server to crash when removing a player's insignia, in a battleground #152
Merged
Conversation
.. insigna, as a Rogue.
Meltie2013
suggested changes
May 8, 2021
MadMaxMangos
added a commit
that referenced
this pull request
Jul 1, 2026
…ration The World AH Repair migration was renumbered Rel22_05_003 -> _005 in the database repo (collision with the merged #152 ItemAndSkinGroupFix). Update the promotion runbook to apply the full chained World sequence (_002.._005) so _005's version gate is satisfied, and confirm the DB tip is 22/5/5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MadMaxMangos
added a commit
that referenced
this pull request
Jul 1, 2026
… externalized AH reads (coordinator) (#414) * feat(mangosd): in-process -t test-mode scaffold (custody groundwork) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mangosd): _exit after -t test to skip Eluna teardown crash (reliable exit code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(db): add CommitTransactionChecked synchronous durable commit Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(mail): SendMailToInTransaction co-commit variant + ordered deferred queue Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): CustodyDeferred online-item rollback lifetime + self-contained header Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(db): harden CommitTransactionChecked (begin-failure, stopped-thread, exception safety) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * build(character): require Character DB 22_05_001 for custody_ledger Bumps the required Character DB version to match the Rel22_05_001 custody_ledger migration (mangoszero/database). Separated from the CommitTransactionChecked infra commit so the shared-DB change carries no feature/migration coupling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): custody_ledger persistent model + CRUD selftest Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): CustodyService primitives + ordered deferred context Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * build(custody): add AH.Service.Custody + CustodyCrashAt + bump MANGOS_WORLD_VER Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): route S1 auction-create through the custody ledger (gated) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): route S2/S3 bid+buyout through the custody ledger (gated) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): route custody buyouts to legacy path (avoid nested-txn abort until Task 10) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): S2 restore-on-failure + fail-closed bid lookup + scalar notify closure (Codex NO-SHIP) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mail): re-resolve deferred mail-push receiver by GUID (I2 scalar-only closure) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): S1 restore-on-failure (refund deposit, drop phantom auction, return item) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): scalar-only command-result closure + active-rows gate + auction-id reuse guard (Codex re-review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): NextBidSeq uses MAX(id) not COUNT to avoid key collision after TTL prune (GLM review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): route S4 win/resolve + buyout through the custody ledger (gated) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): S4 deferred closures scalar-only (drop AuctionEntry* capture) + remove dead GetLiveBidKey Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(custody): mail items survive rollback (dispose only on success) - fixes win-resolution use-after-free (GLM) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(custody): route S5 auction-cancel through the custody ledger (gated) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): terminalize item ledger row on the custody win path (S4) AuctionBidWinningCustody flipped the dep: and bid rows but never the item:<Id> row; SendAuctionWonMailInTransaction moves the item (owner UPDATE or item_instance DELETE) without a ledger flip. So every custody sale/buyout/bot-win left the item:<Id> row CST_RESERVED after the auction row was deleted -> an orphaned non-terminal row, which breaks the reconciliation invariant ("every non-terminal row maps to a live auction") that Tasks 13/14 depend on. Add one in-txn CommitGoldLedgerOnly("item:"+Id) immediately after SendAuctionWonMailInTransaction and before DeleteFromDB. On a win the item always resolves (delivered or destroyed), so the flip is unconditional and safe; it is in-txn so on rollback it rolls back with everything else (no orphan, no false terminal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-custody): route unsold-expiry through checked custody txn (S6) The no-bidder expiry branch of AuctionHouseObject::Update() now routes custody-row auctions through a single checked co-committed transaction (per-auction HasRows gate, X3), mirroring the win-branch gate already at :890-908. S6 is the simplest seam: no bid, no winner, no cut, and NO synchronous in-memory mutation -> no X6 restore (every effect is deferred and does not run on rollback; the auction + item survive intact and the next tick re-resolves cleanly). AuctionHouseMgr.cpp: - SendAuctionExpiredMailInTransaction: co-commit mirror of SendAuctionExpiredMail. Owner-exists branch: defers the online owner's SMSG_AUCTION_OWNER_NOTIFICATION (expired form, sold=false, bid/outbid/bidder all 0) BEFORE the mail push (notify-then-mail, legacy :307), defers RemoveAItem FIRST then zeroes itemGuidLow, and returns the item via CustodyService::DeliverItem (flips "item:<Id>" -> TERMINAL_OK AND co-commits the return mail). Destroy branch: in-txn item_instance DELETE + CommitGoldLedgerOnly("item:"+Id) (the §0 lesson -- no mail means DeliverItem is not called, so the escrow row must be terminalized explicitly), defers RemoveAItem + live delete pItem (X5: destroy only on commit success). - AuctionEntry::ExpireUnsoldCustody: orchestrator mirroring AuctionBidWinningCustody. Returns the item (or destroys it), forfeits the deposit to the house (CommitGoldLedgerOnly("dep:"+Id), house sink, no money/mail), deletes the auction row IN-TXN, and defers the AH-map erase + object delete LAST so `this` stays valid throughout def.run(). - Update() no-bid branch: gated custody path (symmetric with the win branch); legacy path unchanged behind the else. AuctionHouseMgr.h: declare both new methods beside their win-path siblings. Gate-OFF (no custody rows) is byte-identical to legacy. Eluna: legacy expiry fires no AH hook, so S6 fires none either (parity audit in the follow-up Eluna batch). Build green; -t custody green. The SQL differential (online/offline/deleted-account seller) needs the human's quiesced realm (Task 15). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): fire Eluna AH hooks success-only in legacy order across custody seams Audit every custody seam for Eluna AH-hook parity (success-only + legacy sequence position + live entry): S5 Cancel (AuctionHouseHandler.cpp): OnRemove was fired inside if (CommitTransactionChecked()) -- success-only, good -- but BEFORE def.run(), i.e. not at its legacy sequence position among the cancel effects. Move it into a def.effects closure appended AFTER the refund/item-return effects and BEFORE the trailing RemoveAuction/delete closure, so it runs in legacy order during def.run() and still sees a live entry. Captures auctionHouse + auction by value (both live until the final delete effect runs later in the same def.run() pass). Wrapped in #ifdef ENABLE_ELUNA as before. S1 Create: CONFIRMED already correct -- the X6 rollback path returns at :641 before the OnAdd call at :658, so OnAdd fires only on a successful checked commit. No change. S4 Win: legacy AuctionBidWinning (AuctionHouseMgr.cpp:1274-1291) fires NO Eluna AH hook, so the custody win path (AuctionBidWinningCustody) fires none either. No change. S6 Expire: legacy expiry (SendAuctionExpiredMail + the no-bid Update() branch) fires NO Eluna AH hook, so ExpireUnsoldCustody fires none either. No change. Build green; -t custody green. Eluna behavior itself is validated only with an Eluna build + a Lua hook script (manual follow-up); noted here as the brief's stated test caveat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): keep S6 expire rollback-safe (drop synchronous itemGuidLow=0) Adversarial-review fix. SendAuctionExpiredMailInTransaction zeroed auction->itemGuidLow synchronously (before the checked commit) in BOTH the owner-exists and destroy branches. That is an in-memory mutation that SURVIVES a CommitTransactionChecked() rollback -- contradicting the seam's own invariant ("S6 makes NO synchronous in-memory mutation; on rollback the auction + item survive intact and the next tick re-resolves cleanly"). On rollback: def.run() does not fire (the auction stays in the map) but itemGuidLow is now 0; the custody rows revert so HasRows() stays true. The next Update() tick re-enters the seam, GetAItem(0) returns NULL -> early "item not found, and lost" return, yet ExpireUnsoldCustody continues to forfeit the deposit + DeleteFromDB + delete the auction. Net: item never returned to the player, "item:" row orphaned CST_RESERVED, live Item* leaked. Fix: remove both synchronous `auction->itemGuidLow = 0;` writes. Nothing reads itemGuidLow after that point on the success path (the deferred RemoveAItem uses the savedItemGuidLow snapshot; the auction is deleted by the trailing deferred closure), so the field never needs zeroing; on rollback it must stay valid for the next-tick re-resolution. This matches S4's SendAuctionWonMailInTransaction, which never zeroes it either. Also correct the now-stale savedItemGuidLow snapshot comment in both senders. Build green; -t custody green (custody OK / EXIT=0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): fire S5 Eluna OnRemove in exact legacy order (RemoveAuction -> OnRemove -> delete) Adversarial-review refinement (Eluna parity, Minor). The moved S5 OnRemove hook fired BEFORE the trailing RemoveAuction closure, i.e. while the auction was still in the AH map -- whereas the legacy non-custody branch fires OnRemove AFTER RemoveAuction (out of map) and before delete. A Lua OnRemove handler that queried the auction house during the callback would see the auction present on the custody path but absent on legacy. Fold OnRemove into the trailing deferred closure between RemoveAuction and delete self, reproducing the legacy order exactly (RemoveAuction out-of-map -> OnRemove -> delete). Still success-only (the closure runs only in def.run() after a successful checked commit); `self`/auctionHouse stay valid until the delete at the end of the closure. Build green; -t custody green (custody OK / EXIT=0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): terminalize item row on the S6 missing-item path + correct stale itemGuidLow contract docs Independent-review (GLM/Codex) NO-SHIP fixes. IMPORTANT: SendAuctionExpiredMailInTransaction returns early when GetAItem(itemGuidLow) is NULL (item already absent from the live AH cache), WITHOUT terminalizing the "item:<Id>" escrow row -- but ExpireUnsoldCustody still forfeits the deposit and deletes the auction row unconditionally. A custody auction could thus be removed while item:<Id> stays CST_RESERVED, orphaning a non-terminal custody row with no live auction (breaks the reconciliation invariant Tasks 13/14 depend on). Requires a degraded state (cache drift / partial prior corruption) to trigger, but the seam should not introduce a ledger orphan. Fix: flip "item:<Id>" -> TERMINAL_OK ledger-only in the early-return branch (legacy player-visible behavior unchanged -- the item is already lost; item_instance is intentionally NOT touched, matching legacy, since its DB state is unknown when the cache has drifted). MINOR: the function doc comment (AuctionHouseMgr.cpp) and the header declaration comment (AuctionHouseMgr.h) still claimed "itemGuidLow=0 ... are deferred", stale after 4ced3e7 removed the synchronous zero entirely. Both now state that itemGuidLow is intentionally NOT zeroed on the custody path (success deletes the AuctionEntry; rollback needs the original GUID for the next-tick re-resolution), so a future maintainer does not reintroduce either a synchronous or a deferred zero. Build green; -t custody green (custody OK / EXIT=0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-custody): add reconcile sweep and terminal prune Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-custody): add console-only 'ah repair' custody-drift command Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): make repair terminalize drift without minting value Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-custody): wire crash-injection hooks (pre-commit/pre-deferred) into the custody seams Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(ah-custody): add custody differential SQL, crash-injection procedure, operator docs (Task 15) - src/ah-service/tools/custody_diff.sql: the spec Section 8 player-visible projection diff (characters/mail/mail_items/item_instance/auction), count-aware + NULL-safe, parameterized clone schemas, custody_ledger excluded, mail-time normalization toggle. - src/ah-service/tools/custody_crash_test.md: the crash-injection procedure against the now-wired pre-commit/pre-deferred hooks, the forced-rollback (CommitTransactionChecked()==false) case, the full Section 8 matrix, and the promotion gate. - doc/AuctionHouseBot.md: custody operator section (AH.Service.Custody + AH.Service.CustodyCrashAt keys, per-auction HasRows gate, the console-only 'ah repair' terminalize-without-disbursing + force-forfeit escape hatch, promotion checklist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah-custody): address GLM Task-15 review (auction-time normalization + crash-log flush + doc clarifications) I1 (Important): custody_diff.sql had no normalization toggle for auction.time (= expireTime, a wall-clock timestamp set at creation), so an unpinned-clock A/B run would false-positive every created auction in AUCTION_DIFF with no escape. Add @compare_absolute_auction_times (default 0) mirroring the mail toggle: the auction temp tables now NULL out `time` by default, AND the AUCTION_DIFF join compares `time` with the NULL-safe <=> (a plain `=` makes NULL=NULL fail and would report every row -- the gap in the review's suggested minimal fix). Precondition note updated. M1 (Minor): MaybeCrash flushed only stdout before _exit(3); the sLog crash line (written to the log FILE*) could be lost. Use fflush(NULL) to flush all stdio streams so the crash-test log assertion is reliable. M2 (Minor): custody_crash_test.md pre-commit recovery assertion said "the auction is intact", misleading for S1 (create) where no row was ever inserted. Reworded to "pre-seam DB state unchanged -- for S1 no auction row exists; for S2-S6 the existing row is unchanged". M3 (Minor): custody_diff.sql notes that a clean MAIL_DIFF with a dirty MAIL_ITEMS_DIFF means mail.id drifted between clones (re-verify the idle-snapshot precondition). Build green; -t custody green (custody OK / EXIT=0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(ah-custody): add in-game promotion-gate runbook Ties the Section 8 promotion gate together for an operator: prerequisites (build c08a6b7, the two pending mangos0 World migrations, the character0 A/B clones, the custody conf keys), the seeded quiesced fixture, the A/B differential procedure (single-server two-pass + custody_diff.sql invocation + packet-order check), per-seam crash-injection, concurrent-observer, an ah repair drift smoke, and the final promotion checklist + instant rollback note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahbot): forged system owner -- name intercept + constants + default Create AhBotSystemOwner.h (AHBOT_SYSTEM_OWNER_NAME/GUID + helpers), intercept GetPlayerGuidByName so "AuctionHouse" (case-insensitive) resolves to ObjectGuid(HIGHGUID_PLAYER, 0xFFFFFFFE) without a DB round-trip, default AuctionHouseBot.CharacterName to AHBOT_SYSTEM_OWNER_NAME, and add the -t ahowner self-test (TDD: RED before intercept, GREEN after). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahbot): exclude the forged system GUID from the player allocator Wire SkipAhBotSystemOwnerGuid into SetHighestGuids so the char-GUID allocator's next value can never land on the reserved 0xFFFFFFFE sentinel; add belt-and-suspenders contract assertions to -t ahowner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahbot): self-heal the reserved system name on every load LoadReservedPlayersNames() now seeds AHBOT_SYSTEM_OWNER_NAME into m_ReservedNames in-memory on every call (both the early-return/empty-table branch and the main path after the DB loop), so IsReservedName(AuctionHouse) returns true even if the reserved_name table has been wiped. Assertion added to the -t ahowner test verifies this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahbot): reject player mail to the forged system owner Gate added in HandleSendMail after the receiver GUID resolves: if IsAhBotSystemOwnerGuid(rc) is true, send MAIL_ERR_RECIPIENT_NOT_FOUND and return. The bot's own auction proceeds/returns use the internal MailDraft::SendMailTo path which bypasses this handler, so the bot still receives them. Kills COD scams and accidental gifts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahbot): don't loop-spawn the worker on an unresolved bot GUID + operator doc Guard at the top of WorkerSupervisor::Start() returns false immediately when m_botGuid == 0 (AuctionHouseBot.CharacterName unresolved), preventing the supervisor restart loop from running with a useless GUID. The forged system owner always resolves to a non-zero sentinel; 0 means misconfiguration. Adds -t ahowner assertion that Start() refuses with botGuid=0, and adds operator-doc note to doc/AuctionHouseBot.md explaining the reserved system character. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ahworker): relocate ah-service to src/modules/AhWorker (no behavior change) Move the out-of-process worker under the SD3/Eluna module convention so it is submodule-ready. modules/ is added when BUILD_MANGOSD OR BUILD_AH_SERVICE; SD3 and Eluna self-gate on BUILD_MANGOSD AND SCRIPT_LIB_* (they link `game`), while AhWorker self-gates on BUILD_AH_SERVICE and links only ah_ipc + shared. Binary name, install path, and conf template are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ipc): browse wire contract (length-prefixed, deferEluna, strict decode) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ipc): validate inbound frame size on the worker client handler (I8) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahworker): item_instance.data blob decoder for enchant/suffix/charges (I9) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahworker): pure FilterAndPaginate (name filter, page slice, deferEluna set) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahworker): full-parity CanUseItem port as ah_usability lib (rep/honor/proficiency/mount/recipe-set) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(game): AhUsabilityRef evaluator (AhUseResult, D1/D2) + CanUseItem delegates + CanUseItemEluna (D5) Caller audit: CanUseItem(proto) callers that forward specific EQUIP_ERR codes all go through CanUseItem(Item*) which delegates here — codes preserved 1:1 by MapAhUseResult (exhaustive switch + static_assert AHUSE_OK==EQUIP_ERR_OK). Direct proto-overload callers (ViableEquipSlots, NeedBeforeGreed, playerbot, LookupCommands, AuctionHouseMgr) only check == EQUIP_ERR_OK; unaffected. TDD: ahusabilityref RED (LNK2019 before cmake reconfigure), GREEN after; ahowner GREEN (regression). Both exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ahworker): separate ah_usability_difftest exe (worker port vs production ref) (C5/I10) Adds an EXCLUDE_FROM_ALL differential drift-test executable that runs AhUsability::IsUsable (worker) and AhUsabilityRef::Evaluate (reference) over a 15-item x 3-profile battery and asserts 0 drift. AhUsabilityRef.cpp is compiled directly (not via linking game) to surface any boundary violation at build time. Fix: ACE_DOESNT_DEFINE_MAIN must be defined before any include on Windows to prevent ACE's OS_main.h from hijacking `main`. Run: 45/45 agree, 0 drift, exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ah): extract BuildListForKind in-process browse helper (fallback core, C1/I1) Adds AuctionHouseObject::BuildListForKind, a thin dispatcher that routes browse requests to the existing BuildListAuctionItems/BuildListOwnerItems/ BuildListBidderItems (kind 0/1/2) so Task-11 timeout fallback has a single re-entrant call site. For BIDDER the client outbid ids are prepended in CLIENT ORDER before the sweep, matching HandleAuctionListBidderItems exactly. The three inner builders are completely unchanged (behavior-preserving). Also adds AhAppendClientOutbidsForTest free helper (test seam) and -t ahbrowsehelper smoke test locking the order-preserving contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahworker): on-demand Fetch (full parity + blob decode + I3 status) + per-thread browse thread + defer - BrowseHandler::Fetch: cross-DB JOIN (auction x item_instance x world.item_template), AhItemBlob decode, locale overlay (single LEFT JOIN, LocaleConstant suffix V3/D7), COUNT(*) probe to distinguish FETCH_EMPTY vs FETCH_DB_ERROR (I3), D6 proficiency from GetItemProficiencySkill, outbid/curBid/timeLeftMs at full parity. - BrowseThread (ACE_Based::Runnable): explicit BoundedQueue<BrowseQuery>(256,8MB), per-thread MySQL ThreadStart/ThreadEnd (C4), atomic stop, D4 tooMany reply on queue-full via SendFrame (confirmed thread-safe), FETCH_DB_ERROR sends no reply (mangosd TTL fallback), I8 outbound size preflight. - Main.cpp: browse dispatch selftest (RED->GREEN), BrowseThread start after handshake, IPC_BROWSE_QUERY case, guaranteed join before DB/client shutdown (C4). - --selftest: exit 0 confirmed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ahworker): make BrowseThread diagnostic counters atomic (review) m_processed/m_rejected/m_dbErrors were plain uint64 but written from two threads (Submit on IPC-dispatch; run() on browse thread) — latent data race and torn 64-bit read on non-x86. Changed to std::atomic<uint64> with fetch_add(relaxed) at increment sites and load(relaxed) in getters, matching the existing m_stop precedent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah): async browse proxy + pending-map (cap+GUID re-resolve+seq) + bounded Eluna pass + fallback + sub-budget Task 11: mangosd's three AH list handlers become async proxies to the ah-service worker. BrowsePendingMap (MAX_PENDING cap=D3; per-(char,kind) seq + playerGuidLow re-resolve=I4; one GetGameTime clock=M5). World IPC_BROWSE_RESULT branch: tooMany in-process fallback FIRST, then the deferred-Eluna pass (CanUseItemEluna over the full surviving set, no per-tick budget=D5) + re-pagination; TTL sweep. C1 send-fail /timeout in-process fallback via BuildListForKind. AhEnsureRecipeCastMap built at the TOP of LoadAuctionItems before its empty-AH early return (V6/D9). V3 locale LocaleConstant on the wire. I1 bidder client-outbid-first. I5 cross-faction. I6 WS_DRAIN_BROWSE_PER_CALL sub-budget. -t ahbrowsepending + regressions green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(ah): add MaNGOS GPL header to new SP-1 source files ItemInstanceFields, Usability, AhUsabilityRef, and the difftest were created without the standard license header (the briefs only mandated it for BrowseMessages.h). Prepend the verbatim 20-line MaNGOS GPL block; comment-only, BOM-free, no behavior change. ah-service + ah_usability_difftest build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah): Task 11 review -- Allman braces, totalcount assert, BIDDER eluna guard, M5 comment, sweep test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(ah): SP-1 conf floor (2 char DB conns), operator doc, full smoke procedure Raise CharacterDatabaseConnections default 1->2 with reworded M4 comment (pool floor, not a dedicated browse-thread connection). Extend doc/AuctionHouseBot.md with the SP-1 read-model operator note (worker-up forwarding, deferred-Eluna, fallback contract, proficiency parity). Smoke procedure written to Memory tree (untracked). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ahworker): coordinator flip pt1 -- prePaginated wire flag + over-cap pagination SP-1 coordinator flip (worker-mandatory reads). The worker's deferred-Eluna path no longer signals an in-process fallback on overflow; instead it pre-paginates server-side so mangosd never has to serve the browse itself. - BrowseResult gains uint8 prePaginated. tooMany is repurposed to mean ONLY "worker could not serve (queue full / oversize failsafe)" -> mangosd will send "AH unavailable"; prePaginated means ">cap deferred-Eluna: entries ARE the listfrom page, run the Lua veto on the page, do not re-paginate". The two are mutually exclusive by construction. (IPC_BROWSE_RESULT is a MAXLEN rule, so the +1 byte needs no rule change; Decode min-size guard +1.) - FilterAndPaginate collects the listfrom..+50 survivor window alongside the un-paginated set; >cap -> prePaginated=1 + ship just that page (was tooMany=1 + clear entries). The common <=cap path is unchanged (exact in-process parity: mangosd runs Eluna over the full set, then paginates). - D4 (queue saturated) and I8 (oversize failsafe) keep tooMany=1 (now unambiguously "unavailable"); comments updated off "in-process fallback". - selftest: codec round-trip carries prePaginated; the two strict-decode buffers gain the byte; new >cap test asserts prePaginated=1, 50-entry page at listfrom, totalcount=1200. Build clean; ah-service --selftest exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-sp1): coordinator flip pt2 -- mangosd serves "AH unavailable", no in-process read fallback mangosd becomes a pure coordinator for the AH read paths. When the worker is the authority and cannot serve, the player gets "AH temporarily unavailable" instead of an in-process browse. Mutations stay in-process in SP-1 (unaffected). - AuctionHouseHandler.cpp: delete AhServeBrowseInProcess; add AhSendBrowseUnavailable (empty SMSG_AUCTION_*_LIST_RESULT so the client does not hang + a red system line). The 3 read handlers: every worker-active transient failure (register cap / encode oversize / SendFrame fail) now sends unavailable; a NEW `if (sv)` branch sends unavailable when a worker is configured but DOWN. sv==NULL (no worker configured = legacy single-process) still falls through to the in-process path. AhResolveHouse is KEPT (the BIDDER outbid-prepend uses it -- it is not a fallback helper). - World.cpp: forward-decl AhSendBrowseUnavailable (drop AhServeBrowseInProcess); TTL sweep + reply-branch tooMany -> unavailable; the deferred-Eluna pass now splits on prePaginated -- common case runs Eluna over the full set then paginates (exact parity), the >cap edge runs Eluna on the worker's pre- paginated page only and does not re-paginate (decision #2). - BuildListForKind + its -t ahbrowsehelper test are kept (deleted wholesale at SP-6). Build clean; -t ahbrowsepending/ahbrowsehelper/ahusabilityref all OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(ah-sp1): operator doc for the coordinator flip (worker-mandatory reads) The SP-1 section described the removed in-process read fallback. Replace it with the coordinator model: a configured-but-unavailable worker yields "AH temporarily unavailable" (empty list + red chat), nothing served in-process; a worker fault degrades only the AH, never the realm; no-worker builds keep the legacy in-process AH. Add a smoke note for the accepted decision-#2 cosmetic edge (Eluna realm + >1000 usable hits -> a few extra page buttons / sparse deep pages) so it is not mis-flagged during testing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ah-sp1): AH unavailable -> center-screen flash + gate the window-open when worker down Player-facing polish for the coordinator flip: - AhSendUnavailableMessage: one shared "AH temporarily unavailable" responder -- a transient center-screen SMSG_NOTIFICATION flash PLUS the persistent red system chat line. AhSendBrowseUnavailable now routes its message through it (browse/owner/bidder empty-list replies + the World.cpp reply/sweep paths). - SendAuctionHello gate: when a worker is the AH authority and it is down, do NOT open the auction window at all -- send the unavailable message and return. This is the single chokepoint for every open path (auctioneer click, gossip option, .auction GM commands), so the AH stays fully dark while the worker is down. (sv == NULL = no worker configured = legacy single-process -> open normally.) An already-open window cannot be force-closed in 1.12 (no opcode); a window opened while the worker was up then sees the browse-unavailable reply. Build clean; -t ahbrowsepending/ahbrowsehelper/ahusabilityref all OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(ah-sp1): document the center-flash + window-open gate in the unavailable behavior Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ahworker): empty filtered browse must not false-positive as cross-DB failure The FETCH_DB_ERROR detection compared a FILTERED query's legitimate 0-row result against an UNFILTERED `SELECT COUNT(*) FROM auction`, so any empty filtered browse -- an OWNER list with no auctions, a BIDDER list with no bids (e.g. right after a buyout deletes the won auction), a LIST whose item filters match nothing, or an empty house -- was misread as a broken cross-DB JOIN. Pre-flip that fell back to in-process (masking it); post-flip it surfaced as a 10s TTL hang then "AH unavailable" for legitimately-empty browses. Fix: when the filtered query returns 0 rows, probe the BARE cross-DB JOIN (no WHERE filters). If it returns a row, the cross-DB wiring is fine and the query simply matched nothing -> FETCH_EMPTY (serve an empty list immediately, no TTL hang). Only if the bare JOIN is also empty while `auction` is non-empty is it a genuine config failure -> FETCH_DB_ERROR. Verified live: bare JOIN returns 2714 (= full auction count) while buyguid/itemowner filters legitimately return 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(ah-custody): point promotion runbook at renumbered AH Repair migration The World AH Repair migration was renumbered Rel22_05_003 -> _005 in the database repo (collision with the merged #152 ItemAndSkinGroupFix). Update the promotion runbook to apply the full chained World sequence (_002.._005) so _005's version gate is satisfied, and confirm the DB tip is 22/5/5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(ah): move custody_diff.sql to the database repo; repoint docs The AH custody differential is SQL and does not belong in the server repo. Relocated to the mangoszero/database repo at Tools/custody_diff.sql (alongside the other diagnostic SQL there). Repoint the three references (AuctionHouseBot.md, custody_crash_test.md, custody_promotion_runbook.md) to the new location, and fix a sibling stale path (custody_crash_test.md still pointed at the pre-relocation src/ah-service/tools/ path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ah): remediate Codex review of the coordinator flip (F1-F5, F8-doc) Adversarially-verified findings from the SP-1 coordinator-flip code review: - F1 (critical): a configured-but-failed worker Start() nulls the supervisor pointer, and the read handlers keyed unavailable-vs-legacy off that pointer, so a configured worker that failed to start silently reverted to in-process reads and opened the window -- defeating the coordinator invariant. Add a persistent World::IsAhServiceConfigured() authority flag (set before Start(), survives the failed-start teardown) and re-key all four chokepoints (SendAuctionHello + browse/owner/bidder down-branches) off it. - F2 (critical): over-cap deferred-Eluna shipped a worker-paginated page that mangosd veto-filtered on the page only -> short page + pre-veto totalcount. Per the coordinator's correct-or-unavailable contract, the worker now declines the over-cap case (tooMany -> "AH unavailable"). prePaginated is retired entirely: wire field, encode/decode, the mangosd consumer branch, the page-window collection, and both worker selftests. - F4: BrowseResult::Decode now fails closed on semantically-invalid flag combinations (booleans 0/1; tooMany mutually exclusive with elunaPending and carries no entries). - F5: an invalid-UTF-8 search name now sends a terminal SMSG on both browse paths (proxy -> unavailable, legacy -> empty list) instead of a bare return that left the client's browse panel hung. - F3: the cross-DB health probe now mirrors the real query's locales_item join and localized column, so a locale-schema fault reports unavailable instead of slipping through as a false empty result. - F8 (documented, per maintainer decision): Eluna Player:SendAuctionMenu builds MSG_AUCTION_HELLO directly and bypasses the window gate -> recorded as a known limitation in doc/AuctionHouseBot.md rather than patching the third-party Eluna module. Builds clean (RelWithDebInfo); ah-service --selftest green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ahbot): remediate Codex review of the forged system owner (F6, F7) - F6 (critical, hardening): at GUID overflow the generator set the deferred World::StopNow flag but still returned m_nextGuid++, which at the boundary equals the reserved AH-bot system-owner GUID (0xFFFFFFFE for players) -- a real character could be branded with the forged owner's GUID before the shutdown takes effect. Return the top-of-range GUID instead (never the reserved one) and correct the false "cannot collide" comment in AhBotSystemOwner.h. (Reaching the boundary needs ~4.29 billion player GUIDs, so it is effectively unreachable -- minimal safe fix, per maintainer decision.) - F7: an upgraded ahbot.conf carrying an explicit empty AuctionHouseBot.CharacterName bypassed the forged-owner default and left the bot GUID unresolved (0). Coalesce an empty name to AHBOT_SYSTEM_OWNER_NAME so the name-intercept resolves it to the reserved system GUID. Builds clean (RelWithDebInfo). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ahworker): init BrowseQuery profile/mount fields in selftest (GCC/Clang UB) The FilterAndPaginate usable=1 selftest built a BrowseQuery but never initialized q.profile (PlayerProfile scalars) or q.minMountLevel / q.minEpicMountLevel, all of which RowUsable -> AhUsability::IsUsable reads. MSVC happened to read them as usable; GCC/Clang read uninitialized garbage and filtered every row -> "browse selftest FAILED: defer set size=0 (exp 60)" on the branch's first-ever Linux CI run. Pre-existing since SP-1 (Windows-only tested); not introduced by the Codex remediation (RowUsable / the profile / that test were untouched, and the F2 defer-branch change is behavior-identical below the cap). Fix root + symptom: - default-init the wire-struct scalars (PlayerProfile classId/raceId/level/ honorRank; BrowseQuery minMountLevel/minEpicMountLevel) so a default-constructed query can never feed uninitialized memory to the usability check; - give the selftest a capable profile (class 1, level 60) so its level-1 rows are usable deterministically on every platform. Builds clean (RelWithDebInfo); ah-service --selftest green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (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
3 participants
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.
Fixed a bug that caused the server to crash when removing a player's insignia, in a battleground, as a Rogue.
This change is