From a59a544fffa3fbb9a851a469cd8e205fad0081b3 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:14:59 -0700 Subject: [PATCH] Review follow-up C: enforce the extraction resume window The seven-day window was a promise the code never kept. releaseParty fires only on a terminal status, and dnd_expedition.go justified skipping it for 'extracting' by asserting the roster gets cleared when the window lapses and the row flips to 'failed'. Nothing did that: the only extracting -> failed transition lived inside handleResumeCmd, a lazy expiry that runs only when the leader personally types !resume. A leader who quit, forgot, or simply started a different expedition therefore left the row 'extracting' forever. releaseParty never ran, every member stayed seated, and assertNotAdventuring kept refusing them a run of their own. Three holes: - No sweeper. sweepLapsedExtractions reaps every row past completed_at + 7d through completeExpedition, which releases the roster, and DMs the audience. Hourly ticker plus a one-shot at Start() -- a lapse that happened while the bot was down is blocking those members now. The audience is read before the close-out, since completeExpedition disbands the roster expeditionAudience reads. handleResumeCmd's lazy expiry stays, now sharing the extractionLapsed predicate. - The leader could orphan their own party. !expedition start checked only getActiveExpedition, and !resume resolves the newest 'extracting' row, so starting fresh on top of one left it unreachable with its roster still held. It now refuses when that row has a roster; a solo extraction strands nobody, so walking away from one stays normal. - The leader had no way out but to pay. !expedition abandon could not see the extracted row it owns, so closing it meant buying a !resume first. Both it and abandonExpedition now span 'extracting' via ownedLiveExpedition, which sorts active rows first so expeditionCmdStart's run-spawn rollback still tears down the row it just created. This fixes dnd_setup.go for free: a leader who rerolled their character used to strand their whole party. Note the review item this came from (C) was mis-stated: it claimed members and leaders disagree during 'extracting' because expeditionForMember filters status = 'active'. getActiveExpedition filters 'active' too, so they already agree -- and honestly, since nobody is standing in the dungeon. Widening expeditionForMember would have made the member the only player who can see a paused expedition. Not done. Abandoning now DMs the members, and says the true thing when the party is in town rather than the dungeon (supplies already spent, loot already banked). New: dnd_expedition_extract_sweep_test.go, 6 cases. Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa --- internal/plugin/adventure.go | 4 + internal/plugin/dnd_expedition.go | 35 +++- internal/plugin/dnd_expedition_cmd.go | 55 +++++- internal/plugin/dnd_expedition_extract.go | 82 ++++++++- .../dnd_expedition_extract_sweep_test.go | 158 ++++++++++++++++++ 5 files changed, 328 insertions(+), 6 deletions(-) create mode 100644 internal/plugin/dnd_expedition_extract_sweep_test.go diff --git a/internal/plugin/adventure.go b/internal/plugin/adventure.go index 2444037..a164f44 100644 --- a/internal/plugin/adventure.go +++ b/internal/plugin/adventure.go @@ -270,9 +270,13 @@ func (p *AdventurePlugin) Init() error { go p.expeditionRecapTicker() go p.expeditionAmbientTicker() go p.expeditionAutoRunTicker() + go p.expeditionExtractionSweepTicker() // Auto-cashout any arena runs left in 'awaiting' from a prior restart p.arenaCleanupStaleRuns() + // Extractions that lapsed while the bot was down: the roster they hold is + // blocking those members right now, not an hour from now. + p.sweepLapsedExtractions(time.Now().UTC()) return nil } diff --git a/internal/plugin/dnd_expedition.go b/internal/plugin/dnd_expedition.go index 12b6fea..374bb0f 100644 --- a/internal/plugin/dnd_expedition.go +++ b/internal/plugin/dnd_expedition.go @@ -224,6 +224,28 @@ func getActiveExpedition(userID id.UserID) (*Expedition, error) { return e, err } +// ownedLiveExpedition returns the expedition this player owns and has not yet +// finished with — 'active' or 'extracting'. An extracted expedition is not +// over: it holds its roster for the whole resume window (releaseParty is not +// called on it), so the owner is still the only person who can close it. +// +// Active rows sort first, so a leader who owns both — extracted from one, then +// started another — resolves to the one they are standing in. +func ownedLiveExpedition(userID id.UserID) (*Expedition, error) { + row := db.Get().QueryRow(` + SELECT`+expeditionSelectCols+` + FROM dnd_expedition e + WHERE e.user_id = ? + AND e.status IN ('active', 'extracting') + ORDER BY (e.status = 'active') DESC, e.start_date DESC + LIMIT 1`, string(userID)) + e, err := scanExpedition(row) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return e, err +} + // getExpedition fetches by ID regardless of status. Test/admin use. func getExpedition(id string) (*Expedition, error) { row := db.Get().QueryRow(` @@ -385,9 +407,18 @@ func setExpeditionRunID(expID, runID string) error { return err } -// abandonExpedition flags the active expedition as abandoned. Idempotent. +// abandonExpedition flags the player's live expedition as abandoned. Idempotent. +// +// It spans 'extracting' as well as 'active': an extracted expedition still owns +// its roster, so abandoning is the leader's only way to free their party +// without paying to `!resume` first. Character reset (dnd_setup) leans on this +// too — a rerolled leader who left an extracted party behind would otherwise +// strand every member until the sweeper reaped the row. +// +// One row per call, active first, so the run-spawn rollback in expeditionCmdStart +// tears down the expedition it just created rather than an older extracted one. func abandonExpedition(userID id.UserID) error { - e, err := getActiveExpedition(userID) + e, err := ownedLiveExpedition(userID) if err != nil { return err } diff --git a/internal/plugin/dnd_expedition_cmd.go b/internal/plugin/dnd_expedition_cmd.go index 1035c58..dd0289b 100644 --- a/internal/plugin/dnd_expedition_cmd.go +++ b/internal/plugin/dnd_expedition_cmd.go @@ -322,6 +322,26 @@ func (p *AdventurePlugin) expeditionCmdStart(ctx MessageContext, c *DnDCharacter "You're already on expedition in **%s** (Day %d). Finish it or `!expedition abandon` first.", zone.Display, existing.CurrentDay)) } + // A leader who extracted still holds their roster for the resume window, and + // `!resume` only ever reaches the *newest* extracted row. Starting fresh on + // top of one would orphan it: unreachable, un-reapable until the sweeper + // catches it, with every member still seated and refused a run of their own. + // + // Only a row with a roster blocks. A solo extraction strands nobody, so + // walking away from it stays a normal thing to do. + if pending, _ := getResumableExpedition(ctx.Sender); pending != nil { + switch n, err := partySize(pending.ID); { + case extractionLapsed(pending, time.Now().UTC()): + // Past the window — reap it here rather than make them wait an hour + // for the sweeper, and let the new expedition proceed. + _ = completeExpedition(pending.ID, ExpeditionStatusFailed) + case err == nil && n > 1: + zone, _ := getZone(pending.ZoneID) + return p.SendDM(ctx.Sender, fmt.Sprintf( + "You extracted from **%s** on Day %d and your party is still waiting on you. `!resume` to lead them back in, or `!expedition abandon` to let it go — until you do one or the other, none of them can start a run of their own.", + zone.Display, pending.CurrentDay)) + } + } cost := float64(purchase.Cost()) if p.euro == nil { @@ -610,6 +630,15 @@ func (p *AdventurePlugin) expeditionCmdAbandon(ctx MessageContext) error { if err != nil { return p.SendDM(ctx.Sender, "Couldn't read expedition state: "+err.Error()) } + if exp == nil { + // An extracted expedition is still the owner's to close — it holds the + // roster until the resume window lapses. Without this, a leader who + // wanted out had to pay to `!resume` first just to abandon. + if exp, err = getResumableExpedition(ctx.Sender); err != nil { + return p.SendDM(ctx.Sender, "Couldn't read expedition state: "+err.Error()) + } + isLeader = exp != nil + } if exp == nil { return p.SendDM(ctx.Sender, "No active expedition to abandon.") } @@ -619,15 +648,37 @@ func (p *AdventurePlugin) expeditionCmdAbandon(ctx MessageContext) error { "Only your party leader can abandon the expedition. `!expedition leave` to walk out alone.") } zone, _ := getZone(exp.ZoneID) + extracted := exp.Status == ExpeditionStatusExtracting + audience := expeditionAudience(exp) // read before abandonExpedition disbands the roster if err := abandonExpedition(ctx.Sender); err != nil { return p.SendDM(ctx.Sender, "Couldn't abandon: "+err.Error()) } markActedToday(ctx.Sender) _ = retireAllRegionRuns(exp) _ = appendExpeditionLog(exp.ID, exp.CurrentDay, "narrative", "expedition abandoned", "") - if err := p.SendDM(ctx.Sender, fmt.Sprintf( + // An extracted party is standing in town, not in the dungeon: their supplies + // are already spent and their loot is already banked. Say the true thing. + body := fmt.Sprintf( "Expedition in **%s** abandoned on Day %d. Supplies are forfeit. The dungeon remembers.", - zone.Display, exp.CurrentDay)); err != nil { + zone.Display, exp.CurrentDay) + if extracted { + body = fmt.Sprintf( + "You let the expedition in **%s** go. Day %d is where it ends — loot, XP, and coins are kept. The dungeon remembers.", + zone.Display, exp.CurrentDay) + } + // The roster is being disbanded out from under the members; they hear it from + // their leader rather than discovering it the next time a command works again. + for _, uid := range audience { + if uid == ctx.Sender { + continue + } + if err := p.SendDM(uid, fmt.Sprintf( + "Your leader called off the expedition in **%s** on Day %d. You're free to start a run of your own.", + zone.Display, exp.CurrentDay)); err != nil { + slog.Warn("expedition: abandon DM failed", "user", uid, "expedition", exp.ID, "err", err) + } + } + if err := p.SendDM(ctx.Sender, body); err != nil { return err } // Emergence seam: see maybeRollPetArrivalOnEmerge. diff --git a/internal/plugin/dnd_expedition_extract.go b/internal/plugin/dnd_expedition_extract.go index 76beacc..355fd73 100644 --- a/internal/plugin/dnd_expedition_extract.go +++ b/internal/plugin/dnd_expedition_extract.go @@ -30,6 +30,7 @@ import ( const ( extractResumeWindow = 7 * 24 * time.Hour forcedExtractCoinTaxFrac = 0.20 + extractionSweepInterval = time.Hour ) var ( @@ -218,6 +219,82 @@ func getResumableExpedition(userID id.UserID) (*Expedition, error) { return e, err } +// extractionLapsed reports whether an 'extracting' row is past its resume +// window and should be reaped. A NULL completed_at counts as lapsed: the column +// is the only clock the window has, and a row without one can never expire. +func extractionLapsed(e *Expedition, now time.Time) bool { + return e.CompletedAt == nil || now.Sub(*e.CompletedAt) > extractResumeWindow +} + +// loadLapsedExtractions returns every extracted expedition whose resume window +// has closed, regardless of owner. +func loadLapsedExtractions(now time.Time) ([]*Expedition, error) { + rows, err := db.Get().Query(` + SELECT`+expeditionSelectCols+` + FROM dnd_expedition e + WHERE e.status = 'extracting' + AND (e.completed_at IS NULL OR e.completed_at < ?)`, + now.Add(-extractResumeWindow)) + if err != nil { + return nil, err + } + defer rows.Close() + return scanExpeditionRows(rows) +} + +// sweepLapsedExtractions closes out every extraction whose seven-day window has +// run out, which releases its roster. +// +// Without this the window is a promise the code never keeps. `handleResumeCmd` +// expires a lapsed row lazily, but only the *leader* can call `!resume` — so a +// leader who quits, forgets, or simply starts a different expedition leaves the +// row 'extracting' forever, and releaseParty (which fires only on a terminal +// status) never runs. Every member stays seated, and assertNotAdventuring keeps +// refusing them a run of their own. `!expedition leave` is their escape, but a +// player should not have to find it. +// +// The audience is read before the close-out: completeExpedition disbands the +// roster, and expeditionAudience reads that roster. +func (p *AdventurePlugin) sweepLapsedExtractions(now time.Time) { + exps, err := loadLapsedExtractions(now) + if err != nil { + slog.Error("expedition: load lapsed extractions", "err", err) + return + } + for _, e := range exps { + audience := expeditionAudience(e) + if err := completeExpedition(e.ID, ExpeditionStatusFailed); err != nil { + slog.Warn("expedition: expire lapsed extraction", "expedition", e.ID, "err", err) + continue + } + zone, _ := getZone(e.ZoneID) + body := fmt.Sprintf( + "🕯 **The way back has closed — %s**\n\nYour extracted expedition sat past its seven-day resume window, and the dungeon reshaped without you. Day %d is where it ends.\n\nThe loot, XP, and coins you carried out are still yours. `!expedition list` when you're ready for the next one.", + zone.Display, e.CurrentDay) + for _, uid := range audience { + if err := p.SendDM(uid, body); err != nil { + slog.Warn("expedition: lapsed-extraction DM failed", "user", uid, "expedition", e.ID, "err", err) + } + } + } +} + +// expeditionExtractionSweepTicker reaps lapsed extractions hourly. The window is +// seven days, so the cadence only bounds how long a freed member waits to hear +// about it. +func (p *AdventurePlugin) expeditionExtractionSweepTicker() { + ticker := time.NewTicker(extractionSweepInterval) + defer ticker.Stop() + for { + select { + case <-p.stopCh: + return + case <-ticker.C: + p.sweepLapsedExtractions(time.Now().UTC()) + } + } +} + // resumeExpedition flips an extracting row back to 'active' with fresh // supplies. Threat/temporal/region state are preserved as-is. func resumeExpedition(expID string, supplies ExpeditionSupplies) error { @@ -323,8 +400,9 @@ func (p *AdventurePlugin) handleResumeCmd(ctx MessageContext, args string) error if exp == nil { return p.SendDM(ctx.Sender, "No extracted expedition to resume. Use `!expedition start ` to begin a new one.") } - if exp.CompletedAt == nil || time.Since(*exp.CompletedAt) > extractResumeWindow { - // Expire it so it doesn't keep resurfacing. + if extractionLapsed(exp, time.Now().UTC()) { + // Expire it so it doesn't keep resurfacing. The hourly sweeper would get + // here on its own; this keeps the refusal and the reap in one breath. _ = completeExpedition(exp.ID, ExpeditionStatusFailed) return p.SendDM(ctx.Sender, "That extraction is past its 7-day resume window — the dungeon has reshaped without you. Start a new expedition.") diff --git a/internal/plugin/dnd_expedition_extract_sweep_test.go b/internal/plugin/dnd_expedition_extract_sweep_test.go new file mode 100644 index 0000000..66870c9 --- /dev/null +++ b/internal/plugin/dnd_expedition_extract_sweep_test.go @@ -0,0 +1,158 @@ +package plugin + +import ( + "testing" + "time" + + "gogobee/internal/db" + "maunium.net/go/mautrix/id" +) + +// seedExtraction inserts an 'extracting' row whose window closed `age` ago. +// A negative age is a live extraction. +func seedExtraction(t *testing.T, expeditionID string, owner id.UserID, age time.Duration) { + t.Helper() + seedExpedition(t, expeditionID, owner, ExpeditionStatusExtracting) + if _, err := db.Get().Exec( + `UPDATE dnd_expedition SET completed_at = ? WHERE expedition_id = ?`, + time.Now().UTC().Add(-extractResumeWindow-age), expeditionID); err != nil { + t.Fatal(err) + } +} + +func seatedOn(t *testing.T, member id.UserID) string { + t.Helper() + e, err := seatedExpeditionFor(member) + if err != nil { + t.Fatal(err) + } + if e == nil { + return "" + } + return e.ID +} + +// The window is the only thing bounding how long a leader's roster holds their +// party. Nothing else flips an 'extracting' row terminal unless the leader +// personally types !resume, so if the sweep misses, members are stuck forever. +func TestSweepLapsedExtractions_ReleasesTheRosterItStranded(t *testing.T) { + setupEmptyTestDB(t) + leader := id.UserID("@sweep-leader:example.org") + member := id.UserID("@sweep-member:example.org") + + seedExtraction(t, "exp-lapsed", leader, time.Hour) + if err := joinParty("exp-lapsed", member); err != nil { + t.Fatal(err) + } + if got := seatedOn(t, member); got != "exp-lapsed" { + t.Fatalf("member seated on %q before sweep, want exp-lapsed", got) + } + + (&AdventurePlugin{}).sweepLapsedExtractions(time.Now().UTC()) + + e, err := getExpedition("exp-lapsed") + if err != nil { + t.Fatal(err) + } + if e.Status != ExpeditionStatusFailed { + t.Errorf("status = %q, want failed", e.Status) + } + if got := seatedOn(t, member); got != "" { + t.Errorf("member still seated on %q after sweep", got) + } + if n, _ := partySize("exp-lapsed"); n != 1 { + t.Errorf("roster survived the sweep: partySize = %d", n) + } +} + +// The mirror: a leader who extracted an hour ago still has six days to !resume, +// and their party must still be there when they do. +func TestSweepLapsedExtractions_LeavesALiveExtractionAlone(t *testing.T) { + setupEmptyTestDB(t) + leader := id.UserID("@sweep-live-leader:example.org") + member := id.UserID("@sweep-live-member:example.org") + + seedExtraction(t, "exp-live", leader, -6*24*time.Hour) // one day in + if err := joinParty("exp-live", member); err != nil { + t.Fatal(err) + } + + (&AdventurePlugin{}).sweepLapsedExtractions(time.Now().UTC()) + + e, _ := getExpedition("exp-live") + if e.Status != ExpeditionStatusExtracting { + t.Errorf("status = %q, want extracting — the resume window is still open", e.Status) + } + if got := seatedOn(t, member); got != "exp-live" { + t.Errorf("member unseated from a resumable expedition (seated on %q)", got) + } +} + +// completed_at is the only clock the window has. A row without one can never +// lapse, so it must read as already lapsed rather than as immortal. +func TestExtractionLapsed_NullCompletedAtIsLapsed(t *testing.T) { + now := time.Now().UTC() + if !extractionLapsed(&Expedition{}, now) { + t.Error("nil completed_at should read as lapsed") + } + fresh := now.Add(-time.Hour) + if extractionLapsed(&Expedition{CompletedAt: &fresh}, now) { + t.Error("an hour-old extraction is not lapsed") + } + old := now.Add(-extractResumeWindow - time.Second) + if !extractionLapsed(&Expedition{CompletedAt: &old}, now) { + t.Error("a row past the window is lapsed") + } +} + +// A leader must be able to let an extracted expedition go without paying to +// !resume it first — that is the escape hatch !expedition start points them at. +func TestAbandonExpedition_ReachesAnExtractedRowAndFreesTheParty(t *testing.T) { + setupEmptyTestDB(t) + leader := id.UserID("@abandon-extracted-leader:example.org") + member := id.UserID("@abandon-extracted-member:example.org") + + seedExtraction(t, "exp-extracted", leader, -24*time.Hour) // live, resumable + if err := joinParty("exp-extracted", member); err != nil { + t.Fatal(err) + } + + if err := abandonExpedition(leader); err != nil { + t.Fatalf("abandonExpedition on an extracting row: %v", err) + } + e, _ := getExpedition("exp-extracted") + if e.Status != ExpeditionStatusAbandoned { + t.Errorf("status = %q, want abandoned", e.Status) + } + if got := seatedOn(t, member); got != "" { + t.Errorf("member still seated on %q", got) + } +} + +// expeditionCmdStart's run-spawn rollback calls abandonExpedition to tear down +// the row it just created. A leader who also owns an older extracted row must +// not have that one reaped instead. +func TestAbandonExpedition_PrefersTheActiveRowOverAnExtractedOne(t *testing.T) { + setupEmptyTestDB(t) + leader := id.UserID("@abandon-prefers-active:example.org") + + seedExtraction(t, "exp-old-extracted", leader, -24*time.Hour) + seedExpedition(t, "exp-new-active", leader, ExpeditionStatusActive) + + if err := abandonExpedition(leader); err != nil { + t.Fatal(err) + } + if e, _ := getExpedition("exp-new-active"); e.Status != ExpeditionStatusAbandoned { + t.Errorf("active row status = %q, want abandoned", e.Status) + } + if e, _ := getExpedition("exp-old-extracted"); e.Status != ExpeditionStatusExtracting { + t.Errorf("extracted row was collateral damage: status = %q", e.Status) + } +} + +func TestAbandonExpedition_NoLiveRow(t *testing.T) { + setupEmptyTestDB(t) + if err := abandonExpedition(id.UserID("@abandon-nothing:example.org")); err != ErrNoActiveExpedition { + t.Errorf("err = %v, want ErrNoActiveExpedition", err) + } +}