From e2c0c3359bca7ccdf8b422a5d9cf99db5d25863c Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Fri, 10 Jul 2026 18:09:11 -0700 Subject: [PATCH] N6/C3 review fixes: close the killed-boss-resolves-as-survived window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from an adversarial review of the C3 diff, both fixed: 1. (medium) A killing blow commits the shared pool to 0 inline, but the status flip to 'defeated' was deferred until AFTER the multi-second narration stream. In that window a crash/redeploy or the ticker's survive path could resolve a boss the town KILLED as a survival — debiting the pot and paying no bounties. Fix: resolve the defeat BEFORE streaming narration, and add a ticker safety net that resolves any active 0-HP boss as defeated (never falls through to the survive/pot-debit branch). Both guarded by setWorldBossStatus, so still once. 2. (minor) Pool damage was applied before the best-effort contrib/gate write, so a failed upsert let a player refight a pool they had already drained and lose the credit. Fix: write the contribution (which sets the once-per-day gate) BEFORE draining the pool, as a hard error that aborts the bout with the pool untouched. Also corrected the applyWorldBossDamage comment (two concurrent killers CAN both see killed=true; the status guard dedupes the payout — the old comment claimed only one would). New ticker tests cover both resolution paths; suite green, golden byte-identical. Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa --- internal/plugin/adventure_worldboss.go | 40 +++++++++++++----- internal/plugin/adventure_worldboss_test.go | 47 +++++++++++++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/internal/plugin/adventure_worldboss.go b/internal/plugin/adventure_worldboss.go index 1a1d9d0..475a538 100644 --- a/internal/plugin/adventure_worldboss.go +++ b/internal/plugin/adventure_worldboss.go @@ -156,10 +156,11 @@ func insertWorldBoss(name string, tier, hpMax int, startsAt, endsAt time.Time) ( // applyWorldBossDamage subtracts damage from the shared pool atomically, clamped // at zero, only while the boss is still active. Returns the pool's new value and -// whether this hit felled it (crossed to zero on this call). A concurrent hit -// that also crosses zero will see killed=false — only the writer that moved the -// pool to exactly zero, or found it already there, owns resolution; the caller -// re-reads to decide. +// whether the pool is now down (killed). The UPDATE and the re-read are separate +// statements, so two concurrent killers can BOTH observe killed=true — that is +// fine: resolveWorldBossDefeated dedupes on the status='active' guard in +// setWorldBossStatus, so only one payout ever fires. killed is just "should the +// caller attempt resolution". func applyWorldBossDamage(bossID int64, dmg int) (remaining int, killed bool, err error) { if dmg < 0 { dmg = 0 @@ -409,6 +410,14 @@ func (p *AdventurePlugin) worldBossTick() { } now := time.Now().UTC() if boss != nil { + // Safety net: a killing blow commits the pool to 0 inline, but if the + // process died (or was redeployed) before the bout resolved the status, + // the boss is left active/0-HP. Resolve it as the defeat it was — never + // let the survive branch below debit the pot for a boss the town killed. + if boss.HPCurrent <= 0 { + p.resolveWorldBossDefeated(boss) + return + } if now.After(boss.EndsAt) { p.resolveWorldBossSurvived(boss) } @@ -626,6 +635,14 @@ func (p *AdventurePlugin) fightWorldBoss(ctx MessageContext) error { return p.SendDM(ctx.Sender, "The Siege combat hit an error. Try again in a moment.") } + // Resolve a defeat BEFORE streaming the (multi-second) narration. The pool is + // already committed to 0; flipping the status now closes the window where a + // crash or the ticker's survive path could resolve a killed boss as a + // survival and debit the pot instead of paying bounties. + if bout.Killed { + p.resolveWorldBossDefeated(boss) + } + playerName, _ := loadDisplayName(ctx.Sender) if playerName == "" { playerName = "You" @@ -646,10 +663,6 @@ func (p *AdventurePlugin) fightWorldBoss(ctx MessageContext) error { } <-p.sendZoneCombatMessages(ctx.Sender, phaseMessages, footer) - - if bout.Killed { - p.resolveWorldBossDefeated(boss) - } return nil } @@ -688,13 +701,18 @@ func (p *AdventurePlugin) resolveWorldBossBout(userID id.UserID, boss *worldBoss if dmg < 0 { dmg = 0 } + // Record the contribution (which also sets the once-per-day gate) BEFORE + // draining the shared pool. If this write fails we must not have already + // drained the pool — otherwise the player could refight a pool they emptied + // and lose the credit for it. A hard error here aborts the bout; the HP cost + // was already paid, but the pool is untouched and the player can retry. + if err := upsertWorldBossContrib(boss.ID, userID, dmg, dateKey); err != nil { + return worldBossBoutResult{}, err + } remaining, killed, err := applyWorldBossDamage(boss.ID, dmg) if err != nil { return worldBossBoutResult{}, err } - if err := upsertWorldBossContrib(boss.ID, userID, dmg, dateKey); err != nil { - slog.Warn("worldboss: contrib upsert failed", "user", userID, "err", err) - } return worldBossBoutResult{ Damage: dmg, Remaining: remaining, Killed: killed, Battered: battered, Combat: result, }, nil diff --git a/internal/plugin/adventure_worldboss_test.go b/internal/plugin/adventure_worldboss_test.go index d0ba008..8a3c550 100644 --- a/internal/plugin/adventure_worldboss_test.go +++ b/internal/plugin/adventure_worldboss_test.go @@ -368,3 +368,50 @@ func TestResolveWorldBossBout_FellsTinyPool(t *testing.T) { t.Errorf("bout should not self-resolve; status=%q", after.Status) } } + +// TestWorldBossTick_ResolvesZeroHPBossAsDefeated: a killing blow that committed +// the pool to 0 but (crash/redeploy) never resolved the status must be resolved +// as DEFEATED by the ticker net — never fall through to the survive/pot-debit +// path. No contributors here, so resolution never touches euro. +func TestWorldBossTick_ResolvesZeroHPBossAsDefeated(t *testing.T) { + newWorldBossTestDB(t) + now := time.Now().UTC() + communityPotAdd(1000) + bossID, err := insertWorldBoss("Ghost", 3, 100, now, now.Add(worldBossWindow)) + if err != nil { + t.Fatal(err) + } + if _, err := db.Get().Exec(`UPDATE world_boss SET hp_current = 0 WHERE id = ?`, bossID); err != nil { + t.Fatal(err) + } + p := &AdventurePlugin{} + p.worldBossTick() + after, _ := loadWorldBoss(bossID) + if after.Status != "defeated" { + t.Errorf("status = %q, want defeated (ticker safety net)", after.Status) + } + if bal := communityPotBalance(); bal != 1000 { + t.Errorf("pot = %d, want 1000 — a defeat mints, it must not debit the pot", bal) + } +} + +// TestWorldBossTick_ResolvesLapsedBossAsSurvived: a boss whose window lapsed with +// the pool still up survives and loots the pot. +func TestWorldBossTick_ResolvesLapsedBossAsSurvived(t *testing.T) { + newWorldBossTestDB(t) + now := time.Now().UTC() + communityPotAdd(1000) + bossID, err := insertWorldBoss("Titan", 4, 500, now.Add(-worldBossWindow), now.Add(-time.Minute)) + if err != nil { + t.Fatal(err) + } + p := &AdventurePlugin{} + p.worldBossTick() + after, _ := loadWorldBoss(bossID) + if after.Status != "survived" { + t.Errorf("status = %q, want survived", after.Status) + } + if bal := communityPotBalance(); bal != 800 { + t.Errorf("pot = %d, want 800 after 20%% tribute", bal) + } +}