From aac6c3e1270072fe28abb03e07e674c1196818f3 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:45:26 -0700 Subject: [PATCH] adventure: work the five review findings the last pass left open The extract pre-check is gone. It read a snapshot up to two minutes behind and still got the last word, so somebody who set out over Matrix during a lagging roster push was told they weren't on an expedition for a run gogobee would happily have ended. Same call abandon and leave already made: let it through and let rejected_not_running be the answer. The siege_join check stays, because whether a boss is camped outside town is town-wide and runs on a day-or-longer clock, but it now reads one column through SiegeIsCamped instead of loading every defender row and the whole history to look at one flag. The war-room history insert is OR REPLACE. boss_id is the primary key and it was never settled whether gogobee means the siege instance or the boss type by it, so a duplicate pair used to fail the transaction carrying the live boss and the muster too and freeze the war room on the last good snapshot. A dropped history row is the smaller failure; the open question is noted in the schema. offersToUndo's guard didn't cover the case its comment claimed. A gogobee too old to push seats sends a valid blob with no party key, which decodes to the same empty slice as a solo run, and a party member got shown the button that throws away everyone's day. That needs a new field, so whoDetail gains party_known and the flag gates the empty-list branch alone; the branch that reads the viewer's own seat is self-evidencing and keeps working against any sender. gogobee's half is written up in adventure_party_known_flag.md. And an empty offer list no longer claims "you're already out there", which Pete can't actually know from a game box too old to push offers at all. --- adventure_party_known_flag.md | 75 +++++++++++++ code_review_findings_w9.md | 181 ------------------------------- internal/storage/schema.go | 6 + internal/storage/siege.go | 27 ++++- internal/web/orders.go | 38 ++++--- internal/web/orders_test.go | 27 +++-- internal/web/orders_undo_test.go | 34 ++++-- internal/web/siege_test.go | 31 ++++++ internal/web/who.go | 45 +++++--- 9 files changed, 232 insertions(+), 232 deletions(-) create mode 100644 adventure_party_known_flag.md delete mode 100644 code_review_findings_w9.md diff --git a/adventure_party_known_flag.md b/adventure_party_known_flag.md new file mode 100644 index 0000000..88d9560 --- /dev/null +++ b/adventure_party_known_flag.md @@ -0,0 +1,75 @@ +# Wire contract — `party_known` on the public detail blob (gogobee side) + +Status: **Pete side BUILT + TESTED. gogobee side NOT BUILT.** Written 2026-07-24 out +of a code review of the W9 verbs branch. Small: one boolean, set unconditionally, +no new endpoint and no new verdict. Companion to `adventure_expansion_spec.md`. + +## The hole + +`offersToUndo` in `internal/web/who.go` decides which of "Call the whole thing off" +(abandon) and "Leave the party" (leave) an owner's page offers. On a live expedition +with an EMPTY party list it took the "solo run ⇒ this player is the leader" branch, +because a solo run publishes no seats at all. + +But a gogobee too old to push party seats sends a perfectly valid detail blob with no +`party` key, and that decodes to the same empty slice. So a party MEMBER served by an +old game box was shown the leader's button — the one that throws away four people's +day. gogobee refuses it with `rejected_not_leader`, so the live cost was a misleading +offer rather than a lost expedition, but the page was asserting something it could +not know. + +Nil and empty cannot be told apart on the current wire, hence a new field. + +## What gogobee must send + +The public detail blob on the roster push (the one Pete decodes into `whoDetail`) +gains: + +```json +"party_known": true +``` + +**Set it unconditionally**, on every sheet, from any gogobee build that knows what a +party seat is: + +- on a solo run — YES, this is the case the flag exists for; +- when the party list is omitted because there is nobody else — YES; +- when the player is standing in town — YES; +- when the sheet does carry seats — YES, harmless there. + +It is a capability flag about the **sender**, not a fact about the character. It must +never be computed from whether there is a party, whether the run is shared, or +anything else on the sheet. A conditional `party_known` is worse than none: it would +read as "this player is solo" and put the flag straight back into the hole it was +added to close. + +Anything that omits it (old builds, JSON that fails to decode) lands as `false` in +Go, which is the fail-closed answer Pete wants. + +## What Pete already does with it + +- `whoDetail.PartyKnown bool \`json:"party_known"\`` — `internal/web/who.go:47`. +- `offersToUndo`'s third parameter is `partyKnown`; the call site passes + `page.HasDetail && page.Detail.PartyKnown`, so a blob that failed to decode is + still excluded. +- The flag gates the `len(party) == 0` branch **only**. The `len(party) > 0` branch + reads the viewer's own seat and is self-evidencing — a seat that says "member" + cannot be mistaken for leadership — so it keeps working against every sender, + including ones that never set the flag. +- Cases pinned in `internal/web/orders_undo_test.go`. + +## Rollout + +Pete is already deployed-safe: it ships first and costs nothing but one button. Until +gogobee sets the flag, a genuine SOLO leader on a live run does not see the abandon +button on the party branch. They are not stuck — the `self.Resume != nil` clause +still offers it for an extracted run from town, and abandoning over Matrix was never +affected. The moment gogobee ships the flag the button comes back with no Pete change. + +## Done when + +- gogobee sets `party_known: true` on every public detail sheet it builds. +- A solo adventurer on a live expedition sees "Call the whole thing off" on their web + page again. +- A party member on the same build still sees "Leave the party" and never the abandon. +- Delete this file. diff --git a/code_review_findings_w9.md b/code_review_findings_w9.md deleted file mode 100644 index 186c37c..0000000 --- a/code_review_findings_w9.md +++ /dev/null @@ -1,181 +0,0 @@ -# Code review findings + fix plan — w9-verbs-and-polish - -From a `/code-review --fix` pass on the branch diff, 2026-07-24. Two findings were -fixed in that session; the five below were left for a follow-up session, with the -direction chosen by the author. Work them in the order given — items 1/2 and 3 -touch different files and are independent, item 4 is the only one with a -gogobee-side half. - -Already fixed on this branch (don't redo): - -- `internal/web/orders.go` verdict handler — non-`ErrNoSuchAdvOrder` storage - failures now return 500 instead of 400, via a new `storage.ErrBadAdvVerdict`. - A 400 makes gogobee park the row for a human, so a transient SQLite blip used - to permanently strand a resolvable order. -- `internal/web/push_adventure.go` — `advStoryURL` / `advRunOrStoryURL` now - path-escape the guid and run id like every other URL builder does. - ---- - -## 1. Drop the extract pre-check — `internal/web/orders.go:129` - -**Why.** The comment above the pre-checks says they read a snapshot up to two -minutes behind and that "neither is allowed to be the last word", and `who.html` -promises the button works even on an idle-looking mark. But -`if haveEntry && entry.Status != "expedition"` returns 409 and no order is ever -created. A player who set out via Matrix during a lagging roster push (staleness -is tolerated up to 12 minutes) gets told they're not on an expedition for a run -gogobee would happily have ended. - -**Decision: drop it.** Same call `abandon` and `leave` already make one function -down — and the reasoning there (an *extracted* expedition is still abandonable -while its owner reads as idle) applies to extract too. Cost accepted: a genuine -mistake now comes back as a verdict rather than an instant 409. - -**Do.** - -- Delete the `case storage.AdvActionExtract:` arm. Replace it with a comment in - the shape of the `AdvActionAbandon, AdvActionLeave` one in - `resolveAdvOrderParams` — say that the snapshot is stale, that a Matrix - departure can outrun the roster push, and that `rejected_not_running` is the - honest answer. -- With extract gone the `switch req.Action` has one arm left; collapse it to an - `if req.Action == storage.AdvActionSiegeJoin` (see item 2, which rewrites the - body anyway). -- `characterName` is still needed for the insert, so keep the `RosterEntryByToken` - lookup and its error path; only the status test goes. -- Nothing to do client-side: `adventure-actions.js:28` already renders - `rejected_not_running` as "couldn't, you weren't on an expedition", which is - the string the 409 was standing in for. - -**Tests.** `TestActionPreChecksAreCourtesyOnly` in `internal/web/orders_test.go` -pins the old behaviour — its first block asserts `extract` while idle is a 409. -Flip it to 200 and rewrite the doc comment, which currently describes an -asymmetry that no longer exists (both halves are now "let it through" except the -one positive siege case). Consider renaming it to match. Its two siege blocks -stay exactly as they are. - -## 2. Make the siege_join pre-check cheap — `internal/web/orders.go:135` - -**Why.** It calls `storage.LoadSiege()`, which loads every defender row and the -whole siege history, to read one `active` flag, on a pool that is -`MaxOpenConns(1)`. Once per click, so not urgent, but it is a one-column read. - -**Keep the check itself.** Unlike a personal status, "is a boss camped outside -town" is a town-wide fact on a day-or-longer clock, so a two-minute-old copy is -almost never wrong about it — the reason item 1 goes the other way doesn't apply. - -**Do.** - -- Add `SiegeIsCamped() (active, known bool, err error)` to - `internal/storage/siege.go`, next to `LoadSiege`. One - `SELECT active FROM adventure_siege WHERE id = 1`; `sql.ErrNoRows` means - known=false. -- The known/active distinction is load-bearing and already pinned by - `TestActionPreChecksAreCourtesyOnly`: no snapshot at all must queue the order - (a fresh deploy must not have a dead button), only a snapshot that positively - says `active=0` refuses. Keep `LoadSiege`'s doc note about that on the new - function. -- Swap the call in `orders.go`. No behaviour change, so no new test beyond the - existing one continuing to pass. - -## 3. Make the war-room history replace collision-proof — `internal/storage/siege.go:116` - -**Why.** `adventure_siege_history` has `boss_id` as PRIMARY KEY and `ReplaceSiege` -uses a bare `INSERT`, inside the transaction that also writes the live boss and -the muster. Two history rows sharing a `boss_id` fail the whole replace, so the -war room freezes on the previous snapshot indefinitely with no partial -degradation. It is genuinely unclear whether `boss_id` is the siege instance or -the boss type — `SiegeBarForBoss` matches history on `boss_name` + nearest -`ended_at` and its comment says "the same boss comes back month after month", -which reads like type. - -**Decision: don't settle the question, make it survivable.** `INSERT OR REPLACE` -can never lose a row that would otherwise have landed; it just keeps the last of -a colliding pair instead of 500ing the ingest. - -**Do.** - -- `hstmt` becomes `INSERT OR REPLACE INTO adventure_siege_history (...)`. -- Comment why, at the statement: the whole table is deleted and rebuilt from - gogobee's list every push, so a duplicate key is a wire quirk and not data - loss, and a frozen war room is a worse failure than a dropped history row. -- Also note the open question in `schema.go` above the `boss_id INTEGER PRIMARY - KEY` line, so the next reader knows the key's meaning was never confirmed. - -**Tests.** New one in `internal/web/siege_test.go` alongside the existing push -tests: post a snapshot whose history carries two rows with the same `boss_id`, -assert the push succeeds and the war room shows the new live boss — i.e. the -ingest degrades to one history row rather than freezing. - -## 4. Close the party-guard hole — `internal/web/who.go:133` (+ gogobee) - -**Why.** `offersToUndo` receives `page.HasDetail`, which only means "the blob -decoded". Its comment claims the guard also catches "a gogobee too old to push -seats", but such a gogobee pushes a *valid* blob with no `party` key: `HasDetail` -is true, `Party` is nil, and the code takes the "solo ⇒ this player is the -leader" branch and shows a party **member** "Call the whole thing off" — the -exact outcome the comment says it prevents. gogobee refuses with -`rejected_not_leader`, so today's cost is a misleading offer, not a lost -expedition. - -A nil `Party` cannot be told apart from a solo run on the current wire, so this -needs a new field. **Decision: spec the wire change.** - -**Note the asymmetry before writing any code:** only the `len(party) == 0` branch -is unsafe. The `len(party) > 0` branch reads the viewer's *own* seat and is -self-evidencing — it cannot mistake a member for a leader. So gate the empty -branch alone, and the rollout costs nothing but a solo leader's party-branch -abandon in the window before gogobee ships (the `self.Resume != nil` branch below -still covers the extracted case). - -**Pete side.** - -- `whoDetail` gains `PartyKnown bool \`json:"party_known"\`` with a comment - saying what nil-vs-empty could not express. -- `offersToUndo`'s third parameter changes from `haveParty` to `partyKnown`; - pass `page.HasDetail && page.Detail.PartyKnown` at the call site (`who.go:405`) - so a blob that failed to decode is still excluded. -- Move the guard: keep `status == rosterStatusExpedition` on the outer `if`, and - put `partyKnown` on the `len(party) == 0` branch only. -- Rewrite the doc comment — it is currently wrong about what the guard buys, and - the "found by getting a fixture wrong" note should stay but now point at the - real signal. - -**gogobee side (separate repo).** Write it up as a contract the way -`adventure_ask7_equipment_mgmt.md` was: the public detail blob on the roster push -sets `"party_known": true` whenever the sheet was built by a gogobee that knows -about party seats — unconditionally, including on a solo run, and including when -the party list is omitted. It is a capability flag about the *sender*, not a fact -about the character, so it must never be conditional on there being a party. - -**Tests.** `internal/web/orders_undo_test.go` already tables this function. Add -cases: `partyKnown=false` + empty party + expedition ⇒ no abandon (the old -gogobee); `partyKnown=true` + empty party ⇒ abandon (genuine solo); -`partyKnown=false` + a party naming the viewer as member ⇒ leave still offered -(the self-evidencing branch must not regress). - -## 5. Stop asserting a fact Pete can't know — `internal/web/orders.go:210` - -**Why.** `resolveAdvOrderParams` treats `len(detail.Zones) == 0` as proof the -player is on an expedition and says "you're already out there", but an empty -offer list is equally what an out-of-date game box sends. Only reachable via a -hand-crafted request today (with no offers the picker doesn't render), so this is -message honesty, not a live bug. - -**Do.** Keep the refusal, reword it so it describes what Pete actually saw — -something like "nowhere is on offer for you right now" — and adjust the comment -above the branch, which states the "already out there" reading as fact. Then fix -the two places that echo it: the doc comment on -`TestNoZoneOffersMeansAlreadyOut` (`orders_test.go:331`) and, if you want them to -match, nothing else — `adventure-actions.js:33`'s `rejected_busy` string is -gogobee's own verdict and is correct as it stands. - ---- - -## Wrap-up - -- `go build ./...`, `go vet ./...`, `go test ./...` after each item. -- `gofmt -l` already flags `internal/storage/orders.go` on this branch (pre-existing - hand-aligned comment blocks); don't let that fool you into reformatting it. -- Delete this file once the five are done. diff --git a/internal/storage/schema.go b/internal/storage/schema.go index 480d8b4..509eb11 100644 --- a/internal/storage/schema.go +++ b/internal/storage/schema.go @@ -138,6 +138,12 @@ CREATE TABLE IF NOT EXISTS adventure_siege_defenders ( fought_today INTEGER NOT NULL DEFAULT 0 ); +-- Open question, never confirmed with gogobee: whether boss_id identifies the +-- siege instance or the boss TYPE. SiegeBarForBoss matches history on boss_name +-- plus the nearest ended_at and its comment says "the same boss comes back month +-- after month", which reads like a type — in which case this key collides on the +-- second visit. ReplaceSiege inserts OR REPLACE so a collision costs one history +-- row instead of the whole push; settle the meaning before relying on the key. CREATE TABLE IF NOT EXISTS adventure_siege_history ( boss_id INTEGER PRIMARY KEY, boss_name TEXT NOT NULL, diff --git a/internal/storage/siege.go b/internal/storage/siege.go index a17bee5..4a050a4 100644 --- a/internal/storage/siege.go +++ b/internal/storage/siege.go @@ -114,8 +114,14 @@ func ReplaceSiege(s Siege, snapshotAt int64) error { } } + // OR REPLACE, because boss_id is the primary key and a duplicate in gogobee's + // list would otherwise fail this whole transaction — the live boss and the + // muster with it, freezing the war room on the previous snapshot indefinitely. + // The table is deleted and rebuilt from the pushed list every time, so a + // collision is a wire quirk rather than data loss, and keeping the last of a + // colliding pair is a far smaller failure than a war room that stops moving. hstmt, err := tx.Prepare(` - INSERT INTO adventure_siege_history + INSERT OR REPLACE INTO adventure_siege_history (boss_id, boss_name, tier, outcome, hp_remaining, hp_max, defenders, mvp, mvp_fights, ended_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`) if err != nil { @@ -174,6 +180,25 @@ func SiegeBarForBoss(boss string, at int64) (current, max int, ok bool) { return hpCur, hpMax, true } +// SiegeIsCamped answers the one question the siege_join pre-check asks, without +// LoadSiege's defender rows and whole history behind it — a one-column read on a +// pool that is MaxOpenConns(1). +// +// known is false when gogobee has never pushed a war room at all, which is NOT +// the same as a pushed snapshot saying no Siege is camped. The caller has to keep +// the two apart: a fresh deploy that has not been pushed to yet must still queue +// the order rather than show a dead button. +func SiegeIsCamped() (active, known bool, err error) { + err = Get().QueryRow(`SELECT active FROM adventure_siege WHERE id = 1`).Scan(&active) + if err == sql.ErrNoRows { + return false, false, nil + } + if err != nil { + return false, false, err + } + return active, true, nil +} + // LoadSiege returns the war room as last pushed. ok is false when gogobee has // never pushed one at all — distinct from a pushed snapshot that says no Siege // is camped, which is a real answer the page can render. diff --git a/internal/web/orders.go b/internal/web/orders.go index f58222c..dc9d590 100644 --- a/internal/web/orders.go +++ b/internal/web/orders.go @@ -111,10 +111,11 @@ func (s *Server) handleAdvOrder(w http.ResponseWriter, r *http.Request) { return } - // Per-verb pre-checks, all courtesy only. Both read Pete's snapshot copy, which - // is up to two minutes behind the game box, so neither is authoritative and - // neither is allowed to be the last word — a run that ended in that window comes - // back from gogobee as rejected_not_running, which is the honest answer. + // The roster lookup is for the character name the order carries; the one + // surviving pre-check below is courtesy only. Anything read here is Pete's + // snapshot copy, up to two minutes behind the game box, so it is never + // authoritative and is only allowed the last word where being two minutes late + // cannot make it wrong. characterName := "" entry, haveEntry, err := storage.RosterEntryByToken(token) if err != nil { @@ -125,20 +126,25 @@ func (s *Server) handleAdvOrder(w http.ResponseWriter, r *http.Request) { if haveEntry { characterName = entry.Name } - switch req.Action { - case storage.AdvActionExtract: - if haveEntry && entry.Status != "expedition" { - writeAdvOrderError(w, http.StatusConflict, "you're not on an expedition") - return - } - case storage.AdvActionSiegeJoin: - snap, known, err := storage.LoadSiege() + // No pre-check on extract, deliberately, and it is the same call abandon and + // leave make in resolveAdvOrderParams: the mark's status is up to two minutes + // stale here and a Matrix departure can outrun the roster push, so "reads idle" + // would refuse a run gogobee would happily have ended. The cost accepted is + // that a genuine mistake comes back as rejected_not_running rather than as an + // instant refusal, which is the honest answer anyway. + if req.Action == storage.AdvActionSiegeJoin { + // This one stays, because it is not a personal status: whether a boss is + // camped outside town is a town-wide fact on a day-or-longer clock, so a + // two-minute-old copy is almost never wrong about it. Note the known/active + // split — no snapshot at all must queue the order (a fresh deploy must not + // have a dead button); only a snapshot that positively says active=0 refuses. + active, known, err := storage.SiegeIsCamped() if err != nil { slog.Error("orders: siege lookup", "err", err) writeAdvOrderError(w, http.StatusInternalServerError, "internal error") return } - if known && !snap.Active { + if known && !active { writeAdvOrderError(w, http.StatusConflict, "no Siege is camped outside town") return } @@ -207,7 +213,11 @@ func resolveAdvOrderParams(owner, token string, req advOrderReq) (*storage.AdvOr return nil, "pick somewhere to go first" } if len(detail.Zones) == 0 { - return nil, "you're already out there" + // An empty offer list usually means they are already out there, but Pete + // cannot tell that from a game box too old to push offers at all, so say + // only what was actually seen. Unreachable from the page either way — with + // no offers the picker doesn't render — so this is a hand-crafted request. + return nil, "nowhere is on offer for you right now" } for _, z := range detail.Zones { if z.ID != req.Zone { diff --git a/internal/web/orders_test.go b/internal/web/orders_test.go index e71fc85..8ae736c 100644 --- a/internal/web/orders_test.go +++ b/internal/web/orders_test.go @@ -108,15 +108,18 @@ func TestOnlyOneOutstandingOrderPerVerb(t *testing.T) { } } -// TestActionPreChecksAreCourtesyOnly pins both halves of a deliberate asymmetry. -// Pete refuses what its own snapshot says is impossible — but the snapshot is up -// to two minutes old, so the refusal must be cheap and local (a 409 the button -// shows immediately), never a queued order gogobee has to answer. -func TestActionPreChecksAreCourtesyOnly(t *testing.T) { - // Idle mark: extract refused up front. +// TestOnlyTownWideFactsArePreChecked. Pete's copy of the board is up to two +// minutes behind the game box, so what it may refuse locally turns on whether +// being two minutes late could make the answer wrong. A personal status can: +// somebody who set out over Matrix still reads as idle here, and refusing their +// extract would deny a run gogobee would have ended. A boss camped outside town +// cannot: that is town-wide and runs on a day-or-longer clock. +func TestOnlyTownWideFactsArePreChecked(t *testing.T) { + // Idle mark: extract goes through anyway, and rejected_not_running is the + // answer if the mark really was standing in town. s := seedActions(t, "holymachina", "idle") - if w := placeAction(t, s, "holymachina", "extract"); w.Code != 409 { - t.Fatalf("extract while idle = %d, want 409", w.Code) + if w := placeAction(t, s, "holymachina", "extract"); w.Code != 200 { + t.Fatalf("extract while idle = %d, want it queued (%s)", w.Code, w.Body.String()) } // No Siege pushed at all: unknown, not "inactive". Pete has never heard from @@ -328,10 +331,12 @@ func TestExpeditionParamsAreResolvedAgainstTheOwnersOffers(t *testing.T) { } } -// An empty zone list means "already out there", not "nowhere to go" — gogobee -// omits the offers entirely while the adventurer is on an expedition. Refusing +// An empty zone list is a refusal, and the message says only what Pete saw. It +// usually means the adventurer is already out — gogobee omits the offers +// entirely while they are down there — but a game box too old to push offers +// sends the same empty list, so the copy claims nothing about which. Refusing // cheaply here beats a verdict thirty seconds later saying the same thing. -func TestNoZoneOffersMeansAlreadyOut(t *testing.T) { +func TestNoZoneOffersMeansNothingOnOffer(t *testing.T) { s := seedOffers(t, "holymachina", "expedition", storage.PlayerDetail{}) w := placeParams(t, s, "holymachina", advOrderReq{ Action: storage.AdvActionExpedition, Zone: "goblin_warrens", Loadout: "lean", diff --git a/internal/web/orders_undo_test.go b/internal/web/orders_undo_test.go index 2da15c9..b0d33d0 100644 --- a/internal/web/orders_undo_test.go +++ b/internal/web/orders_undo_test.go @@ -36,19 +36,19 @@ func TestOffersToUndoReadsTheViewersOwnSeat(t *testing.T) { cases := []struct { name string token, status string - haveParty bool + partyKnown bool party []partySeat self storage.PlayerDetail abandon, leave, cancel bool }{ { name: "leader of a party is offered the abandon", - token: "tok-josie", status: "expedition", haveParty: true, party: party, + token: "tok-josie", status: "expedition", partyKnown: true, party: party, abandon: true, }, { name: "member of a party is offered the exit, never the abandon", - token: "tok-cam", status: "expedition", haveParty: true, party: party, + token: "tok-cam", status: "expedition", partyKnown: true, party: party, leave: true, }, { @@ -56,7 +56,7 @@ func TestOffersToUndoReadsTheViewersOwnSeat(t *testing.T) { // two seats), so an empty list on a live run means "nobody else", not // "we don't know" — and the one body down there is the leader. name: "solo run is offered the abandon", - token: "tok-josie", status: "expedition", haveParty: true, + token: "tok-josie", status: "expedition", partyKnown: true, abandon: true, }, { @@ -64,19 +64,35 @@ func TestOffersToUndoReadsTheViewersOwnSeat(t *testing.T) { // snapshot we do not understand, and the safe answer is to offer // nothing rather than guess which of the two buttons applies. name: "a party with no seat for the viewer offers nothing", - token: "tok-nobody", status: "expedition", haveParty: true, party: party, + token: "tok-nobody", status: "expedition", partyKnown: true, party: party, }, { // The one that came out of running it. An undecodable public sheet // gives the same empty slice as a solo run, and treating the two alike // offered a party MEMBER the abandon — convincingly, with the rest of - // the page looking fine. haveParty is what tells them apart. + // the page looking fine. name: "a run whose sheet did not decode offers nothing", - token: "tok-cam", status: "expedition", haveParty: false, + token: "tok-cam", status: "expedition", partyKnown: false, + }, + { + // The same empty slice again, this time from a gogobee too old to push + // seats at all. It decodes fine, so only the sender's own flag tells it + // apart from the solo case two rows up. + name: "an empty party from a sender that never pushes seats offers nothing", + token: "tok-cam", status: "expedition", partyKnown: false, party: nil, + }, + { + // The asymmetry: the seat list is self-evidencing, so it keeps working + // against a sender whose capability we cannot confirm. A seat saying + // "member" is not a guess, and refusing the exit here would strand + // somebody in a party for the length of the rollout. + name: "a seated member is offered the exit even without the flag", + token: "tok-cam", status: "expedition", partyKnown: false, party: party, + leave: true, }, { name: "standing in town with nothing open offers nothing", - token: "tok-josie", status: "idle", haveParty: true, party: nil, + token: "tok-josie", status: "idle", partyKnown: true, party: nil, }, { // The case the roster status cannot see: an extracted expedition is @@ -101,7 +117,7 @@ func TestOffersToUndoReadsTheViewersOwnSeat(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - abandon, leave, cancel := offersToUndo(tc.token, tc.status, tc.haveParty, tc.party, tc.self) + abandon, leave, cancel := offersToUndo(tc.token, tc.status, tc.partyKnown, tc.party, tc.self) if abandon != tc.abandon || leave != tc.leave || cancel != tc.cancel { t.Fatalf("offers = abandon:%v leave:%v cancel:%v, want abandon:%v leave:%v cancel:%v", abandon, leave, cancel, tc.abandon, tc.leave, tc.cancel) diff --git a/internal/web/siege_test.go b/internal/web/siege_test.go index 832bdca..e8aa56d 100644 --- a/internal/web/siege_test.go +++ b/internal/web/siege_test.go @@ -236,6 +236,37 @@ func TestSiegeHistoryRendersEndedBar(t *testing.T) { } } +// TestSiegeHistoryCollisionDoesNotFreezeTheWarRoom. boss_id is the history's +// primary key and it is not settled whether gogobee means the siege instance or +// the boss type by it, so two rows can arrive sharing one. Under a bare INSERT +// that failed the transaction carrying the live boss and the muster too, and the +// war room stopped moving on the last good snapshot with nothing to say why. The +// ingest has to degrade to a lost history row instead. +func TestSiegeHistoryCollisionDoesNotFreezeTheWarRoom(t *testing.T) { + s, _ := newAdvServer(t, "tok") + now := time.Now().Unix() + + push := liveSiege(now, 650) + push.Siege.History = []storage.SiegePast{ + {BossID: 3, BossName: "The Ashen Wyrm", Tier: 5, Outcome: "survived", + HPRemaining: 300, HPMax: 1200, Defenders: 3, EndedAt: now - 86400}, + {BossID: 3, BossName: "The Ashen Wyrm", Tier: 5, Outcome: "defeated", + HPRemaining: 0, HPMax: 1200, Defenders: 6, EndedAt: now - 30*86400}, + } + if w := postSiege(t, s, "tok", push); w.Code != 200 { + t.Fatalf("push with a duplicated boss_id = %d, want 200 (%s)", w.Code, w.Body.String()) + } + + v := s.siege() + if !v.Active || v.HPCurrent != 650 { + t.Fatalf("war room = active %v at %d hp, want the pushed live boss — the collision took the whole push down", + v.Active, v.HPCurrent) + } + if len(v.History) != 1 { + t.Errorf("history = %d rows, want 1 (the last of the colliding pair)", len(v.History)) + } +} + // TestSiegeAPIFeedsTheBar. The bar only moves because this endpoint answers, so // the field names it emits are load-bearing: the page's JS reads hp_percent to // set the width and active to decide whether to keep polling at all. diff --git a/internal/web/who.go b/internal/web/who.go index b32b8e3..4b34d6a 100644 --- a/internal/web/who.go +++ b/internal/web/who.go @@ -38,6 +38,13 @@ type whoDetail struct { Map *whoMap `json:"map"` // Party is who else is down there, leader first. Absent on a solo run. Party []partySeat `json:"party"` + // PartyKnown says the sheet was built by a gogobee that knows about party + // seats, which an absent Party cannot: nil means "solo" and "this sender never + // pushes seats" identically, and those two want opposite buttons. It is a + // capability flag about the SENDER, set unconditionally by any gogobee new + // enough — including on a solo run, including when the seat list is omitted — + // so it must never be read as a fact about the character. + PartyKnown bool `json:"party_known"` } // partySeat is one body on a shared expedition as gogobee described it. Kind is @@ -120,28 +127,34 @@ const rosterStatusExpedition = "expedition" // its refusal is the real answer. What this buys is a page that does not put // "Leave the party" in front of somebody standing in town. // -// Nothing new crosses the wire for it. Leadership is already legible in the party -// seats gogobee pushes (W7), and the sitter's standing is already in the babysit -// offer (W5b) — so the two facts the buttons need were both already here. -// haveParty says whether the public detail blob decoded at all, and it is -// load-bearing rather than defensive. An empty seat list means "solo" ONLY when -// we have actually read the sheet; a blob that did not decode — a gogobee too old -// to push seats, a truncated column, a shape change — produces the same empty -// slice, and treating that as solo would offer a party MEMBER the button that -// throws away everyone's day. Found by getting a fixture wrong: the page did -// exactly that, silently and convincingly. -func offersToUndo(token, status string, haveParty bool, party []partySeat, self storage.PlayerDetail) (abandon, leave, cancelSitter bool) { +// Leadership is legible in the party seats gogobee pushes (W7) and the sitter's +// standing is in the babysit offer (W5b), so the facts the buttons need are on +// the wire already — except for one, which nil could not express. partyKnown is +// the sender's "I know about party seats" flag, and it gates the empty-list +// branch alone. An empty list means "solo, so this player is the leader" ONLY +// from a sender that would have listed seats if there were any; from a blob that +// did not decode, or from a gogobee too old to push seats at all, the same empty +// slice would offer a party MEMBER the button that throws away everyone's day. +// Found by getting a fixture wrong: the page did exactly that, silently and +// convincingly. +// +// The asymmetry is deliberate: the len(party) > 0 branch reads the viewer's own +// seat and is self-evidencing — a seat that says "member" cannot be mistaken for +// leadership — so it needs no flag and keeps working against every sender. +func offersToUndo(token, status string, partyKnown bool, party []partySeat, self storage.PlayerDetail) (abandon, leave, cancelSitter bool) { // The sitter first, because it is the only one of the three whose fact does // not go stale: an engagement is a property of the character, not of where // they are standing, so a two-minute-old snapshot is still right about it. cancelSitter = self.Babysit != nil && self.Babysit.Active - if status == rosterStatusExpedition && haveParty { + if status == rosterStatusExpedition { if len(party) == 0 { // A SOLO run publishes no party at all — partySeatViews returns nil - // below two seats — so an empty list here is not "we don't know", it is - // "there is nobody else", which makes this player the leader. - abandon = true + // below two seats — so from a sender that knows about seats an empty list + // is not "we don't know", it is "there is nobody else", which makes this + // player the leader. Without the flag it is exactly "we don't know", and + // the button stays off. + abandon = partyKnown } else { // With a party, offer strictly on the viewer's own seat, and offer // nothing at all if we cannot find it. Guessing in that case would mean @@ -402,7 +415,7 @@ func (s *Server) handleAdventureWho(w http.ResponseWriter, r *http.Request) { } } page.CanAbandon, page.CanLeave, page.CanCancelSitter = - offersToUndo(token, entry.Status, page.HasDetail, page.Detail.Party, self) + offersToUndo(token, entry.Status, page.HasDetail && page.Detail.PartyKnown, page.Detail.Party, self) } } }