mirror of
https://github.com/prosolis/gogobee.git
synced 2026-07-20 10:52:42 +00:00
adventure: fix concurrency + dup hazards in web equip path
Code review of the Ask-7 web equipment-management path surfaced three correctness issues, all fixed here: - applyEquipOrder ran the poll-goroutine equip mutations without the per-user advUserLock that every Matrix-side mutation (!give, !equip, arena, …) holds, so the lock gave no mutual exclusion against the web path. A concurrent !give of the item being equipped could duplicate it. Now takes advUserLock(owner) for the whole apply, matching the DM path. - applyMasterworkEquip evicted the displaced occupant to the pack BEFORE the destructive slot write, so a fault left the piece both worn and in the pack — and the 30s equip poll retry re-evicted it every tick. Now removes the incoming row, writes the slot, then re-packs the occupant last as a best-effort step: once the slot no longer references it, the re-pack cannot duplicate, and a failure is logged not aborted on (the DM confirm handler's tolerance). - PlayerDetail.Balance dropped omitempty: a real €0 balance is an informative fact, not an absent one, and omitting it left the web confirm dialog with no balance to show.
This commit is contained in:
@@ -393,7 +393,9 @@ type PlayerDetail struct {
|
|||||||
// CanTakeOff), not in Equipped, which stays magic-only (the DnD slots).
|
// CanTakeOff), not in Equipped, which stays magic-only (the DnD slots).
|
||||||
Slots []EquipSlotView `json:"slots,omitempty"`
|
Slots []EquipSlotView `json:"slots,omitempty"`
|
||||||
// Balance is the owner's euro balance, for the web's upgrade/repair confirm.
|
// Balance is the owner's euro balance, for the web's upgrade/repair confirm.
|
||||||
Balance float64 `json:"balance,omitempty"`
|
// No omitempty: a €0 balance is a real, informative fact (a broke player), not
|
||||||
|
// an absent one — dropping it would let the confirm dialog show a stale amount.
|
||||||
|
Balance float64 `json:"balance"`
|
||||||
}
|
}
|
||||||
|
|
||||||
// EquipSlotView is one of the 5 standard equipment slots, carrying what the web
|
// EquipSlotView is one of the 5 standard equipment slots, carrying what the web
|
||||||
|
|||||||
@@ -122,6 +122,14 @@ func (p *AdventurePlugin) fulfilEquipOrder(ctx context.Context, order peteclient
|
|||||||
// a human note for Pete, or retry=true for a transient fault that should leave the
|
// a human note for Pete, or retry=true for a transient fault that should leave the
|
||||||
// order pending. It records nothing and pushes nothing — the caller does both.
|
// order pending. It records nothing and pushes nothing — the caller does both.
|
||||||
func (p *AdventurePlugin) applyEquipOrder(owner id.UserID, order peteclient.EquipOrder) (status, detail string, retry bool) {
|
func (p *AdventurePlugin) applyEquipOrder(owner id.UserID, order peteclient.EquipOrder) (status, detail string, retry bool) {
|
||||||
|
// Serialize against the owner's own Matrix-side mutations (!give, !equip, !sell,
|
||||||
|
// arena, …), all of which hold this same per-user lock. Without it the poll
|
||||||
|
// goroutine's equip could interleave with a concurrent !give of the very item it
|
||||||
|
// resolved — the duplication the DM equip confirm takes this lock to prevent.
|
||||||
|
userMu := p.advUserLock(owner)
|
||||||
|
userMu.Lock()
|
||||||
|
defer userMu.Unlock()
|
||||||
|
|
||||||
switch order.Action {
|
switch order.Action {
|
||||||
case "equip":
|
case "equip":
|
||||||
inv, err := loadAdvInventory(owner)
|
inv, err := loadAdvInventory(owner)
|
||||||
|
|||||||
@@ -48,10 +48,14 @@ type mwEquipOutcome struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// applyMasterworkEquip wears one masterwork/arena backpack piece into its standard
|
// applyMasterworkEquip wears one masterwork/arena backpack piece into its standard
|
||||||
// slot, mirroring the magic path's anti-duplication ordering: evict any special
|
// slot. Ordering is anti-duplication AND safe under the equip poll's 30s retry:
|
||||||
// occupant, then remove the incoming row FIRST, then write the slot, restoring the
|
// remove the incoming row FIRST (restoring it on a save fault), then write the
|
||||||
// row on a save fault. One implementation of that ordering per family of gear —
|
// slot, and only THEN evict any displaced special occupant back to the pack. The
|
||||||
// the DM confirm handler (adventure_masterwork.go) is the message-carrying twin.
|
// eviction comes last, once the slot no longer references the occupant, so it can
|
||||||
|
// never mint a duplicate; and it is best-effort — a failure there is logged, not
|
||||||
|
// aborted on, the same tolerance the DM confirm handler (adventure_masterwork.go)
|
||||||
|
// lives with. Aborting after the slot write would strand a completed equip for a
|
||||||
|
// retry that re-evicts the occupant on every tick.
|
||||||
func applyMasterworkEquip(uid id.UserID, it AdvItem) (mwEquipOutcome, error) {
|
func applyMasterworkEquip(uid id.UserID, it AdvItem) (mwEquipOutcome, error) {
|
||||||
if it.Slot == "" || (it.Type != "MasterworkGear" && it.Type != "ArenaGear") {
|
if it.Slot == "" || (it.Type != "MasterworkGear" && it.Type != "ArenaGear") {
|
||||||
return mwEquipOutcome{}, errItemNotEquippable
|
return mwEquipOutcome{}, errItemNotEquippable
|
||||||
@@ -74,19 +78,18 @@ func applyMasterworkEquip(uid id.UserID, it AdvItem) (mwEquipOutcome, error) {
|
|||||||
return mwEquipOutcome{}, errEquipDowngrade
|
return mwEquipOutcome{}, errEquipDowngrade
|
||||||
}
|
}
|
||||||
|
|
||||||
// Evict a special occupant back to the pack. A plain shop-tier occupant is not
|
// Capture the special occupant to evict, if any, BEFORE the slot write below
|
||||||
// an item — it is just the slot's tier — so it is overwritten, not evicted, the
|
// mutates cur in place. A plain shop-tier occupant is not an item — it is just
|
||||||
// same as the DM confirm handler and the shop.
|
// the slot's tier — so it is overwritten, not evicted, the same as the DM confirm
|
||||||
var swappedBack string
|
// handler and the shop. The actual re-pack happens after the slot write (below),
|
||||||
|
// so it can never duplicate the piece.
|
||||||
|
var evicted *AdvItem
|
||||||
if cur != nil && (cur.Masterwork || cur.ArenaTier > 0) {
|
if cur != nil && (cur.Masterwork || cur.ArenaTier > 0) {
|
||||||
old := AdvItem{Name: cur.Name, Type: "MasterworkGear", Tier: cur.Tier, Slot: slot, SkillSource: cur.SkillSource}
|
old := AdvItem{Name: cur.Name, Type: "MasterworkGear", Tier: cur.Tier, Slot: slot, SkillSource: cur.SkillSource}
|
||||||
if cur.ArenaTier > 0 {
|
if cur.ArenaTier > 0 {
|
||||||
old.Type = "ArenaGear"
|
old.Type = "ArenaGear"
|
||||||
}
|
}
|
||||||
if err := addAdvInventoryItem(uid, old); err != nil {
|
evicted = &old
|
||||||
return mwEquipOutcome{}, err
|
|
||||||
}
|
|
||||||
swappedBack = cur.Name
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Destructive op first: pull the incoming row before writing the slot, so a save
|
// Destructive op first: pull the incoming row before writing the slot, so a save
|
||||||
@@ -124,6 +127,20 @@ func applyMasterworkEquip(uid id.UserID, it AdvItem) (mwEquipOutcome, error) {
|
|||||||
}
|
}
|
||||||
return mwEquipOutcome{}, err
|
return mwEquipOutcome{}, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The slot now holds the incoming piece, so the former occupant is referenced
|
||||||
|
// nowhere — re-packing it now cannot duplicate it. Best-effort: a failure is a
|
||||||
|
// bounded, non-compounding loss we log rather than abort on, since the equip has
|
||||||
|
// already succeeded and aborting would re-run (and re-evict) on the next poll.
|
||||||
|
var swappedBack string
|
||||||
|
if evicted != nil {
|
||||||
|
if err := addAdvInventoryItem(uid, *evicted); err != nil {
|
||||||
|
slog.Error("equip: masterwork equipped but evicted piece failed to return to pack",
|
||||||
|
"user", uid, "evicted", evicted.Name, "err", err)
|
||||||
|
} else {
|
||||||
|
swappedBack = evicted.Name
|
||||||
|
}
|
||||||
|
}
|
||||||
return mwEquipOutcome{Name: it.Name, Slot: slot, Tier: it.Tier, Arena: it.Type == "ArenaGear", SwappedBack: swappedBack}, nil
|
return mwEquipOutcome{Name: it.Name, Slot: slot, Tier: it.Tier, Arena: it.Type == "ArenaGear", SwappedBack: swappedBack}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user