Review fixes: party close-out, member soft-lock, supply race, elite loot, season crown

Five bugs found reviewing n1-restoration end to end.

beginCombatTurn settles any phase the engine owes before reading whose turn it
is. That settle can end the fight — and the old code then answered "you're not
in a fight" and returned. The terminal status was already persisted, so nothing
ever paid the party out: no XP, no loot, no death recorded, no run teardown. The
reaper cannot recover it either, because listExpiredCombatSessions filters on
status='active'. Close the fight out there, the way the !fight start path and
the reaper already do.

A party member was permanently soft-locked when their leader extracted and never
resumed. seatedExpeditionFor (the guard) spans 'extracting'; expeditionForMember
(what !expedition leave resolved through) saw only 'active'. So the member was
refused any new adventure by the guard and told "No active expedition" by the
command the guard points them at, with nothing sweeping stale rows and only the
leader able to clear one. Resolve the exit through the same lookup as the gate.

updateSupplies overwrites supplies_json wholesale, and expeditionCmdAccept folded
a member's packs onto a snapshot read before the coin debit, unlocked. Handlers
run one goroutine per event, so two invitees accepting genuinely interleave and
one member's packs vanish. advUserLock cannot help — it is keyed by sender, so
racing members take different mutexes. Add advExpeditionLock and re-read the pool
under it. Closes accept-vs-accept; the six other updateSupplies callers still
race and are written up separately.

runHarvestInterrupt picked an elite enemy and elite narration off a local `elite`
flag, then passed a hardcoded false as isElite to closeOutZoneWin. dropZoneLoot
gates masterwork on isBoss||isElite, so beating an elite interrupt skipped the
masterwork roll and took standard treasure weight — while the same elite fought
via !zone paid out correctly.

arenaSeasonRollover marked its job complete even when recordArenaSeasonTitle
failed, and JobCompleted short-circuits every later run for that quarter, so a
transient SQLite BUSY lost the crown forever. Defer completion on failure; the
insert is ON CONFLICT DO NOTHING against PRIMARY KEY (season, kind) and a past
season's data is frozen, so the retry is safe.

Also: drop dead partySurvivors, collapse the zoneCombatRoster alias into
fightRoster, route partyCasualtyLine through joinNames, fold four copies of the
expedition column projection into expeditionSelectCols, stop replyDM sending a
blank DM, and correct two doc comments describing a path that no longer exists.

Deliberately not fixed, with reasons, in gogobee_code_review_followups.md — most
notably that both turn-based close-outs skip grantCombatAchievements and
persistDnDPostCombatSubclass, which the auto-resolve paths run.

internal/plugin/adventure.go

@@ -28,6 +28,7 @@ type AdventurePlugin struct {
 	dmToPlayer     map[id.RoomID]id.UserID
 	pending        sync.Map // userID string -> *advPendingInteraction
 	userLocks      sync.Map // userID string -> *sync.Mutex
+	expLocks       sync.Map // expedition ID string -> *sync.Mutex
 	dmRemindedDate sync.Map // userID string -> "2006-01-02" date string
 	dmMenuSentAt   sync.Map // userID string -> time.Time (last time actionable menu was DM'd)
 	arenaDeadlines sync.Map // userID string -> time.Time (auto-cashout deadline)
@@ -70,6 +71,16 @@ func (p *AdventurePlugin) advUserLock(userID id.UserID) *sync.Mutex {
 	return val.(*sync.Mutex)
 }
 
+// advExpeditionLock returns a per-expedition mutex, for state a whole party
+// shares rather than one player. advUserLock cannot stand in: it is keyed by
+// sender, so two members racing the same expedition row take two different
+// mutexes and exclude nobody. Handlers run one goroutine per event, so any
+// read-modify-write of the shared supply pool needs this.
+func (p *AdventurePlugin) advExpeditionLock(expID string) *sync.Mutex {
+	val, _ := p.expLocks.LoadOrStore(expID, &sync.Mutex{})
+	return val.(*sync.Mutex)
+}
+
 // advDMResponseWindow is the default window for active state-holding
 // prompts (shop/blacksmith/hospital/masterwork/treasure confirms).
 // Passive overnight-tolerant prompts (pet arrival, flavor DMs) use

internal/plugin/adventure_arena_season.go

@@ -186,6 +186,7 @@ func (p *AdventurePlugin) arenaSeasonRollover(now time.Time) {
 	}
 
 	var lines []string
+	failed := false
 	for _, kind := range []string{arenaTitleEarnings, arenaTitleStreak} {
 		uid, value, ok := arenaSeasonChampion(kind, start, end)
 		if !ok {
@@ -193,6 +194,7 @@ func (p *AdventurePlugin) arenaSeasonRollover(now time.Time) {
 		}
 		if err := recordArenaSeasonTitle(season, kind, uid, value, now); err != nil {
 			slog.Error("arena season: record title failed", "season", season, "kind", kind, "err", err)
+			failed = true
 			continue // don't announce a crown we failed to persist
 		}
 		name, _ := loadDisplayName(uid)
@@ -206,6 +208,14 @@ func (p *AdventurePlugin) arenaSeasonRollover(now time.Time) {
 		}
 	}
 
+	// Marking the job done is what stops the next midnight from retrying, so a
+	// crown we failed to persist must not mark it. recordArenaSeasonTitle is
+	// idempotent on (season, kind), and a past season's data is frozen, so the
+	// retry re-derives the same champions and no-ops the ones already stored.
+	if failed {
+		slog.Warn("arena season: deferring completion after title failure", "season", season)
+		return
+	}
 	db.MarkJobCompleted(jobName, season)
 	if len(lines) == 0 {
 		slog.Info("arena season: closed with no entrants", "season", season)

internal/plugin/combat_cmd.go

@@ -34,7 +34,9 @@ func encounterIDForRoom(roomIdx int) string {
 // state mutations (HP, XP, threat, run-clear) still happen; only the
 // narration is dropped. Non-silent callers (manual !fight) are unchanged.
 func (p *AdventurePlugin) replyDM(ctx MessageContext, text string) error {
-	if ctx.Silent {
+	// An empty body means the caller already answered the player another way —
+	// a party fan-out, say. Sending it would post a blank DM.
+	if ctx.Silent || text == "" {
 		return nil
 	}
 	return p.SendDM(ctx.Sender, text)

internal/plugin/combat_engine_party_test.go

@@ -3,8 +3,6 @@ package plugin
 import (
 	"math/rand/v2"
 	"testing"
-
-	"maunium.net/go/mautrix/id"
 )
 
 // seededRNG gives each test its own deterministic stream, so none of these can
@@ -197,8 +195,8 @@ func TestEventsForSeat_PartitionsTheLog(t *testing.T) {
 }
 
 // A party that wins with a casualty is a win, and the casualty is still a
-// casualty. partySurvivors is what the close-out splits on.
+// casualty: survival is read per seat off HP, never off the fight's outcome.
-func TestPartySurvivors_SplitsOnHPNotOnOutcome(t *testing.T) {
+func TestAnySurvivor_ReadsHPNotOutcome(t *testing.T) {
 	res := PartyCombatResult{
 		PlayerWon: true,
 		Seats: []CombatResult{
@@ -207,14 +205,13 @@ func TestPartySurvivors_SplitsOnHPNotOnOutcome(t *testing.T) {
 			{PlayerEndHP: 3},
 		},
 	}
-	up, down := partySurvivors(res, []id.UserID{"@a:x", "@b:x", "@c:x"})
-	if len(up) != 2 || len(down) != 1 {
-		t.Fatalf("split %d up / %d down, want 2/1", len(up), len(down))
-	}
-	if down[0].String() != "@b:x" {
-		t.Fatalf("wrong casualty: %s", down[0])
-	}
 	if !res.AnySurvivor() {
 		t.Fatal("AnySurvivor said nobody lived")
 	}
+
+	// A won fight nobody walked away from is still nobody walking away.
+	wiped := PartyCombatResult{PlayerWon: true, Seats: []CombatResult{{PlayerEndHP: 0}}}
+	if wiped.AnySurvivor() {
+		t.Fatal("AnySurvivor counted a downed seat as standing")
+	}
 }

internal/plugin/combat_party_turn.go

@@ -147,7 +147,18 @@ func (p *AdventurePlugin) beginCombatTurn(sender id.UserID, noFightMsg string) (
 		return fail("Couldn't resolve the round: " + serr.Error())
 	}
 	if !sess.IsActive() {
-		return fail(noFightMsg)
+		// The owed phase was lethal: the settle above ended the fight. The
+		// terminal status is already persisted, and the reaper only scans for
+		// active sessions, so this is the last chance anyone has to pay the
+		// party out. Close it here rather than answering "you're not in a
+		// fight" over a win nobody was credited for.
+		ct := &combatTurn{sess: sess, players: players, enemy: enemy, seat: seat, uid: sender}
+		outcomes := p.closePartyRound(ct)
+		if !ct.isParty() {
+			return fail(outcomes[0])
+		}
+		p.announcePartyRound(ct, nil, "", outcomes)
+		return fail("")
 	}
 
 	acting, waiting := actingSeat(sess, players, enemy)

internal/plugin/dnd_expedition.go

@@ -193,21 +193,29 @@ func startExpedition(userID id.UserID, zoneID ZoneID, runID string, supplies Exp
 	return exp, nil
 }
 
+// expeditionSelectCols is the column list scanExpedition reads, in the order it
+// reads them. scanExpedition's Scan is positional, so every query that feeds it
+// must project exactly this — hence one constant rather than a copy per query.
+// The `e.` alias means a query can JOIN expedition_party without ambiguity; a
+// query with no JOIN just aliases dnd_expedition to e.
+const expeditionSelectCols = `
+		e.expedition_id, e.user_id, e.zone_id, e.run_id, e.status,
+		e.start_date, e.current_day, e.current_region, e.boss_defeated,
+		e.supplies_json, e.camp_json, e.threat_level, e.threat_siege,
+		e.threat_events, e.temporal_stack, e.region_state,
+		e.xp_earned, e.coins_earned, e.gm_mood,
+		e.last_briefing_at, e.last_recap_at, e.last_ambient_kind,
+		e.last_activity, e.completed_at`
+
 // getActiveExpedition returns the player's in-flight expedition, or (nil, nil).
 // 'extracting' rows are post-extraction (resumable) — see getResumableExpedition.
 func getActiveExpedition(userID id.UserID) (*Expedition, error) {
 	row := db.Get().QueryRow(`
-		SELECT expedition_id, user_id, zone_id, run_id, status,
+		SELECT`+expeditionSelectCols+`
-		       start_date, current_day, current_region, boss_defeated,
+		  FROM dnd_expedition e
-		       supplies_json, camp_json, threat_level, threat_siege,
+		 WHERE e.user_id = ?
-		       threat_events, temporal_stack, region_state,
+		   AND e.status = 'active'
-		       xp_earned, coins_earned, gm_mood,
+		 ORDER BY e.start_date DESC
-		       last_briefing_at, last_recap_at, last_ambient_kind,
-		       last_activity, completed_at
-		  FROM dnd_expedition
-		 WHERE user_id = ?
-		   AND status = 'active'
-		 ORDER BY start_date DESC
 		 LIMIT 1`, string(userID))
 	e, err := scanExpedition(row)
 	if errors.Is(err, sql.ErrNoRows) {
@@ -219,14 +227,8 @@ func getActiveExpedition(userID id.UserID) (*Expedition, error) {
 // getExpedition fetches by ID regardless of status. Test/admin use.
 func getExpedition(id string) (*Expedition, error) {
 	row := db.Get().QueryRow(`
-		SELECT expedition_id, user_id, zone_id, run_id, status,
+		SELECT`+expeditionSelectCols+`
-		       start_date, current_day, current_region, boss_defeated,
+		  FROM dnd_expedition e WHERE e.expedition_id = ?`, id)
-		       supplies_json, camp_json, threat_level, threat_siege,
-		       threat_events, temporal_stack, region_state,
-		       xp_earned, coins_earned, gm_mood,
-		       last_briefing_at, last_recap_at, last_ambient_kind,
-		       last_activity, completed_at
-		  FROM dnd_expedition WHERE expedition_id = ?`, id)
 	e, err := scanExpedition(row)
 	if errors.Is(err, sql.ErrNoRows) {
 		return nil, nil

internal/plugin/dnd_expedition_combat.go

@@ -149,7 +149,7 @@ func (p *AdventurePlugin) runHarvestInterrupt(
 	// P6e: the party fights the interrupt. The surprise nick above stays on the
 	// leader — it is one free swing at whoever is walking point.
 	pres, seated, err := p.runZoneCombatRoster(
-		zoneCombatRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
+		fightRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
 	if err != nil {
 		return fmt.Sprintf("_(Interrupt combat error: %v.)_", err), false
 	}
@@ -221,7 +221,7 @@ func (p *AdventurePlugin) runHarvestInterrupt(
 	}
 	b.WriteString(fmt.Sprintf("✅ **%s** down (HP %d→%d / %d).",
 		monster.Name, preCombatHP, postHP, maxHP))
-	drop, downed := p.closeOutZoneWin(pres, seated, zone, monster, false, false, "expedition")
+	drop, downed := p.closeOutZoneWin(pres, seated, zone, monster, false, elite, "expedition")
 	if drop != "" {
 		b.WriteString("\n")
 		b.WriteString(drop)
@@ -492,7 +492,7 @@ func (p *AdventurePlugin) tryPatrolEncounter(
 		return
 	}
 	pres, seated, rerr := p.runZoneCombatRoster(
-		zoneCombatRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
+		fightRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
 	if rerr != nil {
 		err = rerr
 		return

internal/plugin/dnd_expedition_supplies.go

@@ -333,6 +333,9 @@ func addSupplyPurchase(s ExpeditionSupplies, p SupplyPurchase) ExpeditionSupplie
 // (re-validated under HP buff, shipped).
 const phase5BDailyBurnRatePct = 50
 
+// applyDailyBurn is the solo-rate burn. Since P6b folded the roster in, the live
+// callers all go through applyExpeditionDailyBurn instead; this survives as the
+// fixture the supply tests pin the solo rate against.
 func applyDailyBurn(s ExpeditionSupplies, harshActive, siege bool) (ExpeditionSupplies, float32) {
 	return applyDailyBurnP(s, harshActive, siege, phase5BDailyBurnRatePct)
 }
@@ -379,7 +382,8 @@ func expeditionBurnRatePct(expeditionID string) int {
 // applyDailyBurnP is the rate-parameterized form used by the Phase 3-B
 // sim harness lever sweep. burnRatePct == 0 means "use live" (100%);
 // any positive value scales the final per-day burn by that percent
-// (e.g. 50 = half burn). Live callers always go through applyDailyBurn.
+// (e.g. 50 = half burn). Live callers reach it through applyExpeditionDailyBurn,
+// which supplies the roster-scaled rate.
 // See gogobee_expedition_difficulty.md Phase 3-B.
 func applyDailyBurnP(s ExpeditionSupplies, harshActive, siege bool, burnRatePct int) (ExpeditionSupplies, float32) {
 	burn := s.DailyBurn

internal/plugin/dnd_zone_cmd.go

@@ -1047,7 +1047,7 @@ func (p *AdventurePlugin) resolveCombatRoom(userID id.UserID, run *DungeonRun, z
 	// Seats[0] is the leader — their view drives this narration, which is the
 	// leader's stream. The mood scan reads the whole fight's log.
 	pres, seated, rerr := p.runZoneCombatRoster(
-		zoneCombatRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
+		fightRoster(userID), monster, int(zone.Tier), nil, run.DMMood)
 	if rerr != nil {
 		err = rerr
 		return

internal/plugin/expedition_party.go

@@ -253,13 +253,7 @@ func disbandParty(expeditionID string) error {
 // activeExpeditionFor provides.
 func expeditionForMember(userID id.UserID) (*Expedition, error) {
 	row := db.Get().QueryRow(`
-		SELECT e.expedition_id, e.user_id, e.zone_id, e.run_id, e.status,
+		SELECT`+expeditionSelectCols+`
-		       e.start_date, e.current_day, e.current_region, e.boss_defeated,
-		       e.supplies_json, e.camp_json, e.threat_level, e.threat_siege,
-		       e.threat_events, e.temporal_stack, e.region_state,
-		       e.xp_earned, e.coins_earned, e.gm_mood,
-		       e.last_briefing_at, e.last_recap_at, e.last_ambient_kind,
-		       e.last_activity, e.completed_at
 		  FROM dnd_expedition e
 		  JOIN expedition_party p ON p.expedition_id = e.expedition_id
 		 WHERE p.user_id = ? AND p.role <> 'leader' AND e.status = 'active'

internal/plugin/expedition_party_cmd.go

@@ -178,8 +178,22 @@ func (p *AdventurePlugin) expeditionCmdAccept(ctx MessageContext, c *DnDCharacte
 	if isHol, _ := isHolidayToday(); isHol {
 		suppliesPurchase.StandardPacks++
 	}
-	pooled := addSupplyPurchase(exp.Supplies, suppliesPurchase)
+	// updateSupplies overwrites supplies_json wholesale, so the pool has to be
-	if err := updateSupplies(exp.ID, pooled); err != nil {
+	// re-read under the expedition's own lock: `exp` was fetched before the coin
+	// debit, and a second invitee accepting — or the leader's day-burn tick —
+	// may have rewritten the row since. Folding onto that stale snapshot would
+	// silently drop their packs or resurrect spent SU.
+	expMu := p.advExpeditionLock(exp.ID)
+	expMu.Lock()
+	fresh, err := getExpedition(exp.ID)
+	if err != nil || fresh == nil {
+		expMu.Unlock()
+		return p.SendDM(ctx.Sender, "You're in, but your supplies didn't reach the pool.")
+	}
+	pooled := addSupplyPurchase(fresh.Supplies, suppliesPurchase)
+	err = updateSupplies(exp.ID, pooled)
+	expMu.Unlock()
+	if err != nil {
 		return p.SendDM(ctx.Sender, "You're in, but your supplies didn't reach the pool: "+err.Error())
 	}
 
@@ -261,6 +275,20 @@ func (p *AdventurePlugin) expeditionCmdParty(ctx MessageContext) error {
 // expeditionCmdLeave walks a member out. The leader cannot leave — their row is
 // the expedition — so they are pointed at `!extract`, which ends it for all.
 func (p *AdventurePlugin) expeditionCmdLeave(ctx MessageContext) error {
+	// Resolve the seat the way the guards that trap them do. seatedExpeditionFor
+	// spans `extracting`, which activeExpeditionFor does not: a leader who
+	// extracts and never resumes would otherwise leave their members seated —
+	// refused a new adventure by the guard, and told "no active expedition" by
+	// the very command the guard points them at. The exit has to see every state
+	// the gate sees. It already excludes leaders, so they fall through below.
+	seated, err := seatedExpeditionFor(ctx.Sender)
+	if err != nil {
+		return p.SendDM(ctx.Sender, "Couldn't read expedition state: "+err.Error())
+	}
+	if seated != nil {
+		return p.leaveSeatedParty(ctx, seated)
+	}
+
 	exp, isLeader, err := activeExpeditionFor(ctx.Sender)
 	if err != nil {
 		return p.SendDM(ctx.Sender, "Couldn't read expedition state: "+err.Error())
@@ -272,6 +300,12 @@ func (p *AdventurePlugin) expeditionCmdLeave(ctx MessageContext) error {
 		return p.SendDM(ctx.Sender,
 			"You're leading this one — `!extract` ends it for everyone, or `!expedition abandon` to walk away from it.")
 	}
+	return p.leaveSeatedParty(ctx, exp)
+}
+
+// leaveSeatedParty unseats a member and tells both ends. Shared by the two ways
+// a member's seat resolves: the `extracting` limbo and the plain active party.
+func (p *AdventurePlugin) leaveSeatedParty(ctx MessageContext, exp *Expedition) error {
 	if err := leaveParty(exp.ID, ctx.Sender); err != nil {
 		return p.SendDM(ctx.Sender, "Couldn't leave: "+err.Error())
 	}

internal/plugin/expedition_party_resolve.go

@@ -86,13 +86,7 @@ func isPartyMember(userID id.UserID) bool {
 // wins every activeZoneRunFor lookup once the leader resumes.
 func seatedExpeditionFor(userID id.UserID) (*Expedition, error) {
 	row := db.Get().QueryRow(`
-		SELECT e.expedition_id, e.user_id, e.zone_id, e.run_id, e.status,
+		SELECT`+expeditionSelectCols+`
-		       e.start_date, e.current_day, e.current_region, e.boss_defeated,
-		       e.supplies_json, e.camp_json, e.threat_level, e.threat_siege,
-		       e.threat_events, e.temporal_stack, e.region_state,
-		       e.xp_earned, e.coins_earned, e.gm_mood,
-		       e.last_briefing_at, e.last_recap_at, e.last_ambient_kind,
-		       e.last_activity, e.completed_at
 		  FROM dnd_expedition e
 		  JOIN expedition_party p ON p.expedition_id = e.expedition_id
 		 WHERE p.user_id = ? AND p.role <> 'leader'

internal/plugin/zone_combat_party.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"log/slog"
 	"math/rand/v2"
-	"strings"
 	"time"
 
 	"maunium.net/go/mautrix/id"
@@ -163,19 +162,6 @@ func (p *AdventurePlugin) runZoneCombatRoster(
 // the same rule, for the same reason.
 func zoneCombatRoster(walker id.UserID) []id.UserID { return fightRoster(walker) }
 
-// partySurvivors splits a resolved roster into who is still standing and who
-// went down, in seating order.
-func partySurvivors(res PartyCombatResult, seated []id.UserID) (up, down []id.UserID) {
-	for i, uid := range seated {
-		if res.Seats[i].PlayerEndHP > 0 {
-			up = append(up, uid)
-		} else {
-			down = append(down, uid)
-		}
-	}
-	return up, down
-}
-
 // closeOutZoneWin hands loot to every seat still standing and the hospital to
 // every seat that isn't. It returns the leader's own loot line — the only one
 // that belongs in the leader's room narration — and who went down.
@@ -242,8 +228,5 @@ func partyCasualtyLine(downed []id.UserID) string {
 		}
 		names = append(names, "**"+name+"**")
 	}
-	if len(names) == 1 {
+	return "💀 " + joinNames(names) + " went down."
-		return "💀 " + names[0] + " went down."
-	}
-	return "💀 " + strings.Join(names[:len(names)-1], ", ") + " and " + names[len(names)-1] + " went down."
 }

internal/plugin/zone_combat_party_casualty_test.go

@@ -0,0 +1,38 @@
+package plugin
+
+import (
+	"testing"
+
+	"gogobee/internal/db"
+	"maunium.net/go/mautrix/id"
+)
+
+// partyCasualtyLine leans on joinNames so a party's casualty DM punctuates its
+// name list the same way every other multi-name line the bot sends does.
+func TestPartyCasualtyLine_JoinsNamesLikeTheRestOfTheBot(t *testing.T) {
+	dir := t.TempDir()
+	db.Close()
+	if err := db.Init(dir); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(db.Close)
+
+	// No player_meta rows, so every name falls back to its localpart.
+	cases := []struct {
+		name   string
+		downed []id.UserID
+		want   string
+	}{
+		{"nobody fell", nil, ""},
+		{"one", []id.UserID{"@ana:x"}, "💀 **ana** went down."},
+		{"two", []id.UserID{"@ana:x", "@bo:x"}, "💀 **ana** and **bo** went down."},
+		{"three", []id.UserID{"@ana:x", "@bo:x", "@cy:x"}, "💀 **ana**, **bo**, and **cy** went down."},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := partyCasualtyLine(tc.downed); got != tc.want {
+				t.Fatalf("partyCasualtyLine(%v) = %q, want %q", tc.downed, got, tc.want)
+			}
+		})
+	}
+}