diff --git a/internal/api/handlers/watch_together.go b/internal/api/handlers/watch_together.go index 69486025..0fc64add 100644 --- a/internal/api/handlers/watch_together.go +++ b/internal/api/handlers/watch_together.go @@ -367,6 +367,9 @@ func (h *WatchTogetherHandler) HandleSelectRoomItem(w http.ResponseWriter, r *ht }) if err != nil { switch { + case errors.Is(err, watchtogether.ErrVoteRoomSelection): + writeError(w, http.StatusConflict, "vote_room_selection", + "This room votes for what plays; start the winning suggestion instead") case errors.Is(err, watchtogether.ErrRoomForbidden): writeError(w, http.StatusForbidden, "forbidden", "Only the host can start or switch room playback") case errors.Is(err, watchtogether.ErrRoomNotFound): @@ -568,6 +571,18 @@ func (h *WatchTogetherHandler) HandlePromoteSuggestion(w http.ResponseWriter, r switch { case errors.Is(err, watchtogether.ErrRoomForbidden): writeError(w, http.StatusForbidden, "forbidden", "Only the host can promote a suggestion") + case errors.Is(err, watchtogether.ErrNotVoteWinner): + writeError(w, http.StatusConflict, "not_vote_winner", + "This room votes for what plays; start the title that is winning") + case errors.Is(err, watchtogether.ErrNoVotesCast): + writeError(w, http.StatusConflict, "no_votes_cast", + "Nobody has voted yet") + // Promoting the winner is the sanctioned way into a vote room's + // selection, so this should not escape the service. Mapped anyway so a + // regression in that path reads as a conflict rather than a 500. + case errors.Is(err, watchtogether.ErrVoteRoomSelection): + writeError(w, http.StatusConflict, "vote_room_selection", + "This room votes for what plays; start the winning suggestion instead") case errors.Is(err, watchtogether.ErrSuggestionNotFound): writeError(w, http.StatusNotFound, "not_found", "Suggestion not found") case errors.Is(err, watchtogether.ErrRoomNotFound): diff --git a/internal/watchtogether/service.go b/internal/watchtogether/service.go index f1253cb7..e3111219 100644 --- a/internal/watchtogether/service.go +++ b/internal/watchtogether/service.go @@ -26,8 +26,17 @@ var ( ErrConnectionNotAttached = errors.New("watch together session is not attached") ErrInvalidSelection = errors.New("watch together selection is invalid") ErrSuggestionNotFound = errors.New("watch together suggestion not found") - ErrDuplicateVote = errors.New("watch together already voted") - ErrNotVoted = errors.New("watch together not voted") + // ErrNotVoteWinner is returned when a vote-mode room is asked to promote + // something other than the title the room actually voted for. + ErrNotVoteWinner = errors.New("watch together suggestion is not the vote winner") + // ErrNoVotesCast is returned when a vote-mode room is asked to start before + // anyone has voted: there is no winner to promote yet. + ErrNoVotesCast = errors.New("watch together room has no votes yet") + // ErrVoteRoomSelection is returned when a vote room's selection is set + // directly instead of through the vote. + ErrVoteRoomSelection = errors.New("watch together vote room selects by vote") + ErrDuplicateVote = errors.New("watch together already voted") + ErrNotVoted = errors.New("watch together not voted") ) const ( @@ -161,6 +170,23 @@ type Service struct { rooms map[string]*liveRoom } +// defaultHostDisconnectTTL is how long a room survives its host's socket going +// away without an explicit leave. +// +// This is not "how long before we assume the host left" — an explicit leave and +// an explicit close both tear the room down immediately, so this timer only +// ever covers a host who has NOT said they are going. At 15s it treated any +// transient drop as a departure: a host who backgrounded the app, walked +// through a tunnel, or simply navigated somewhere the client did not hold the +// socket open lost the room for everybody, mid-conversation, with a +// "host_left" nobody could explain. +// +// Two minutes is long enough to survive a reconnect, an app switch, or a +// client that drops the socket while its user browses for something to +// suggest; short enough that a genuinely departed host does not leave a room +// sitting open all evening. The janitor still reaps idle rooms independently. +const defaultHostDisconnectTTL = 2 * time.Minute + func NewService( repo RoomStore, sessions RoomSessionLookup, @@ -176,7 +202,7 @@ func NewService( files: files, selectionResolver: selectionResolver, profileNames: profileNames, - hostDisconnectTTL: 15 * time.Second, + hostDisconnectTTL: defaultHostDisconnectTTL, now: func() time.Time { return time.Now().UTC() }, @@ -826,12 +852,30 @@ func (s *Service) UpdatePolicy( return snapshot, nil } +// SelectItem sets what the room plays at the host's direct request. In a vote +// room that request is refused: the vote decides, and PromoteSuggestion is the +// only way in. func (s *Service) SelectItem( ctx context.Context, roomID string, userID int, profileID string, input SelectItemInput, +) (Snapshot, error) { + return s.selectItem(ctx, roomID, userID, profileID, input, false) +} + +// selectItem carries out a selection. viaVote is set only by PromoteSuggestion +// once it has confirmed the suggestion is the room's winner — that call has +// already satisfied the vote, so gating it here would leave a vote room with no +// way at all to start playback. +func (s *Service) selectItem( + ctx context.Context, + roomID string, + userID int, + profileID string, + input SelectItemInput, + viaVote bool, ) (Snapshot, error) { if strings.TrimSpace(input.ContentID) == "" { return Snapshot{}, ErrInvalidSelection @@ -861,6 +905,15 @@ func (s *Service) SelectItem( s.mu.Unlock() return Snapshot{}, ErrRoomForbidden } + // A vote room decides by tally, and a direct selection is the other door + // into the room's selection. Gating only PromoteSuggestion would leave the + // host able to set any title directly and bypass the vote entirely, which + // makes the counts on everyone else's screen decoration. Once the room is + // voting, the winner is the only way in. + if !viaVote && live.room.SelectionMode == RoomSelectionModeVote { + s.mu.Unlock() + return Snapshot{}, ErrVoteRoomSelection + } if live.room.Phase == RoomPhaseEnded { s.mu.Unlock() return Snapshot{}, ErrRoomClosed @@ -1873,9 +1926,62 @@ func (s *Service) PromoteSuggestion( return Snapshot{}, ErrSuggestionNotFound } - return s.SelectItem(ctx, roomID, userID, profileID, SelectItemInput{ + // In a vote room the host starts the winner; they do not get to overrule it. + // Being able to promote any suggestion would make "vote" host_pick with + // extra steps, and the tally on everyone else's screen would be a lie. + // + // The winner is read here and the selection commits a moment later, so a + // vote landing in between can start a title that has just stopped being the + // head of the tally. That is deliberate: the host pressed start on the + // standings they and the room could see, and a vote arriving during the + // round trip should not retroactively overrule the press. Closing the window + // would mean holding the room lock across a suggestion-store read, which + // stalls every other room for a race whose worst case is off by one vote. + s.mu.Lock() + isVoteRoom := live.room.SelectionMode == RoomSelectionModeVote + s.mu.Unlock() + if isVoteRoom { + winner, err := s.VoteWinner(ctx, roomID) + if err != nil { + return Snapshot{}, err + } + if winner.ID != suggestion.ID { + return Snapshot{}, ErrNotVoteWinner + } + } + + return s.selectItem(ctx, roomID, userID, profileID, SelectItemInput{ ContentID: suggestion.ContentID, - }) + }, isVoteRoom) +} + +// VoteWinner returns the suggestion a vote-mode room has settled on. +// +// The repository already orders by vote_count DESC, created_at ASC, so the +// winner is the head of the list and ties resolve to whoever suggested first — +// deterministic, and it does not reward re-suggesting the same title. +// +// A room where nobody has voted has no winner. Returning the oldest suggestion +// there would let a host "start the vote winner" for a vote that never +// happened, which is exactly the confusion this mode exists to avoid. +func (s *Service) VoteWinner(ctx context.Context, roomID string) (Suggestion, error) { + if s == nil || s.suggestions == nil { + return Suggestion{}, fmt.Errorf("watch together suggestions unavailable") + } + suggestions, err := s.suggestions.ListSuggestions(ctx, roomID, "") + if err != nil { + return Suggestion{}, err + } + return winnerFrom(suggestions) +} + +// winnerFrom picks the winner out of an already-ordered suggestion list. Split +// out so the rule can be tested without a database. +func winnerFrom(ordered []Suggestion) (Suggestion, error) { + if len(ordered) == 0 || ordered[0].VoteCount <= 0 { + return Suggestion{}, ErrNoVotesCast + } + return ordered[0], nil } func (s *Service) prepareSuggestionDispatchesLocked(live *liveRoom, suggestions []Suggestion) []snapshotDispatch { diff --git a/internal/watchtogether/vote_selection_test.go b/internal/watchtogether/vote_selection_test.go new file mode 100644 index 00000000..7b79a01c --- /dev/null +++ b/internal/watchtogether/vote_selection_test.go @@ -0,0 +1,142 @@ +package watchtogether + +import ( + "context" + "errors" + "testing" + "time" +) + +// stubSuggestions serves an already-ordered list, the way the repository's +// "vote_count DESC, created_at ASC" query does. +type stubSuggestions struct { + ordered []Suggestion +} + +func (s *stubSuggestions) CreateSuggestion(context.Context, Suggestion) (*Suggestion, error) { + return nil, errors.New("not used") +} + +func (s *stubSuggestions) GetSuggestion(_ context.Context, id string) (*Suggestion, error) { + for _, suggestion := range s.ordered { + if suggestion.ID == id { + found := suggestion + return &found, nil + } + } + return nil, ErrSuggestionNotFound +} + +func (s *stubSuggestions) ListSuggestions(context.Context, string, string) ([]Suggestion, error) { + out := make([]Suggestion, len(s.ordered)) + copy(out, s.ordered) + return out, nil +} + +func (s *stubSuggestions) DeleteSuggestion(context.Context, string) error { return nil } +func (s *stubSuggestions) AddVote(context.Context, string, string) error { return nil } +func (s *stubSuggestions) RemoveVote(context.Context, string, string) error { + return nil +} + +func newVoteRoomService(t *testing.T, mode RoomSelectionMode, ordered []Suggestion) (*Service, *stubRepo) { + t.Helper() + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + repo := &stubRepo{room: Room{ + ID: "room-1", + Code: "ROOM1234", + JoinToken: "TOKEN1234", + HostUserID: 7, + HostProfileID: "host", + Phase: RoomPhaseLobby, + SelectionMode: mode, + GuestControlPolicy: GuestControlPolicyHostOnly, + IsPaused: true, + AnchorUpdatedAt: now, + Generation: 1, + CreatedAt: now, + }} + service := newServiceForTest( + now, + repo, + &stubSessions{}, + &stubFiles{}, + &stubSelectionResolver{resolved: &ResolvedSelection{ContentID: "movie-winner"}}, + ) + service.suggestions = &stubSuggestions{ordered: ordered} + t.Cleanup(service.Close) + return service, repo +} + +func voteRoomSuggestions() []Suggestion { + return []Suggestion{ + {ID: "winner", RoomID: "room-1", ContentID: "movie-winner", Title: "Heat", VoteCount: 3}, + {ID: "runner-up", RoomID: "room-1", ContentID: "movie-other", Title: "Alien", VoteCount: 1}, + } +} + +// The gate on direct selection and the gate on promotion sit on the same code +// path, so a vote room can very easily end up with no way in at all. This is +// the test that catches that: promoting the winner must still start playback. +func TestPromotingTheWinnerStartsAVoteRoom(t *testing.T) { + service, _ := newVoteRoomService(t, RoomSelectionModeVote, voteRoomSuggestions()) + + snapshot, err := service.PromoteSuggestion(context.Background(), "room-1", "winner", 7, "host") + if err != nil { + t.Fatalf("PromoteSuggestion() error = %v, want the vote winner to start", err) + } + if snapshot.Phase != RoomPhasePlaying { + t.Fatalf("phase = %q, want %q", snapshot.Phase, RoomPhasePlaying) + } + if snapshot.SelectedContentID == nil || *snapshot.SelectedContentID != "movie-winner" { + t.Fatalf("selected content = %v, want movie-winner", snapshot.SelectedContentID) + } +} + +func TestPromotingSomethingOtherThanTheWinnerIsRefused(t *testing.T) { + service, _ := newVoteRoomService(t, RoomSelectionModeVote, voteRoomSuggestions()) + + _, err := service.PromoteSuggestion(context.Background(), "room-1", "runner-up", 7, "host") + if !errors.Is(err, ErrNotVoteWinner) { + t.Fatalf("PromoteSuggestion() error = %v, want ErrNotVoteWinner", err) + } +} + +func TestPromotingBeforeAnyoneVotesIsRefused(t *testing.T) { + unvoted := []Suggestion{{ID: "a", RoomID: "room-1", ContentID: "movie-a", VoteCount: 0}} + service, _ := newVoteRoomService(t, RoomSelectionModeVote, unvoted) + + _, err := service.PromoteSuggestion(context.Background(), "room-1", "a", 7, "host") + if !errors.Is(err, ErrNoVotesCast) { + t.Fatalf("PromoteSuggestion() error = %v, want ErrNoVotesCast", err) + } +} + +// The host bypassing the tally with a direct selection would make the counts on +// everyone else's screen decoration. +func TestDirectSelectionIsRefusedInAVoteRoom(t *testing.T) { + service, _ := newVoteRoomService(t, RoomSelectionModeVote, voteRoomSuggestions()) + + _, err := service.SelectItem(context.Background(), "room-1", 7, "host", SelectItemInput{ + ContentID: "movie-winner", + }) + if !errors.Is(err, ErrVoteRoomSelection) { + t.Fatalf("SelectItem() error = %v, want ErrVoteRoomSelection", err) + } +} + +// Neither gate applies to a host_pick room: the host picks, votes are not part +// of the mode, and an unvoted suggestion is still promotable. +func TestHostPickRoomIsUntouchedByTheVoteGates(t *testing.T) { + unvoted := []Suggestion{{ID: "a", RoomID: "room-1", ContentID: "movie-winner", VoteCount: 0}} + service, _ := newVoteRoomService(t, RoomSelectionModeHostPick, unvoted) + + if _, err := service.SelectItem(context.Background(), "room-1", 7, "host", SelectItemInput{ + ContentID: "movie-winner", + }); err != nil { + t.Fatalf("SelectItem() error = %v, want a host_pick room to select directly", err) + } + if _, err := service.PromoteSuggestion(context.Background(), "room-1", "a", 7, "host"); err != nil { + t.Fatalf("PromoteSuggestion() error = %v, want a host_pick room to promote freely", err) + } +} diff --git a/internal/watchtogether/vote_winner_test.go b/internal/watchtogether/vote_winner_test.go new file mode 100644 index 00000000..b700b881 --- /dev/null +++ b/internal/watchtogether/vote_winner_test.go @@ -0,0 +1,51 @@ +package watchtogether + +import "testing" + +// The winner is the head of the repository's ordering (vote_count DESC, +// created_at ASC). These pin the rule the ordering encodes, so a change to the +// query that breaks it fails here rather than silently starting the wrong film. +func TestVoteWinnerIsTheHeadOfTheOrdering(t *testing.T) { + ordered := []Suggestion{ + {ID: "b", Title: "Heat", VoteCount: 3}, + {ID: "a", Title: "Alien", VoteCount: 1}, + {ID: "c", Title: "Ronin", VoteCount: 0}, + } + + winner, err := winnerFrom(ordered) + if err != nil { + t.Fatalf("no winner: %v", err) + } + if winner.ID != "b" { + t.Fatalf("winner = %q, want the most-voted", winner.ID) + } +} + +// A vote nobody cast has no winner. Promoting the oldest suggestion there would +// let a host "start the winner" of a vote that never happened — precisely the +// confusion this mode exists to remove. +func TestVoteWinnerRefusesWhenNobodyHasVoted(t *testing.T) { + if _, err := winnerFrom(nil); err != ErrNoVotesCast { + t.Fatalf("empty room: err = %v, want ErrNoVotesCast", err) + } + unvoted := []Suggestion{{ID: "a", VoteCount: 0}, {ID: "b", VoteCount: 0}} + if _, err := winnerFrom(unvoted); err != ErrNoVotesCast { + t.Fatalf("no votes: err = %v, want ErrNoVotesCast", err) + } +} + +// Ties resolve to whoever suggested first, which the ordering gives us for +// free — and which means re-suggesting the same title cannot jump the queue. +func TestVoteWinnerBreaksTiesByOrder(t *testing.T) { + tied := []Suggestion{ + {ID: "first", VoteCount: 2}, + {ID: "second", VoteCount: 2}, + } + winner, err := winnerFrom(tied) + if err != nil { + t.Fatal(err) + } + if winner.ID != "first" { + t.Fatalf("tie went to %q, want the earlier suggestion", winner.ID) + } +}