From 172beb99effe4bbd310150dbeb6aa682bbcca291 Mon Sep 17 00:00:00 2001 From: RXWatcher Date: Mon, 27 Jul 2026 04:01:31 +0200 Subject: [PATCH] fix(watch-together): stop a dropped socket reading as the host leaving (#487) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(watch-together): make vote rooms actually vote selection_mode has been stored, normalized and published since the feature landed, and nothing has ever read it. A "vote" room behaved exactly like a host_pick one: members could suggest and vote, the tally was recorded and broadcast, and then the host promoted whatever they liked regardless of it. In a vote room the host now starts the winner rather than choosing it. Promoting anything other than the leading suggestion is refused, because being able to overrule the tally makes the mode host_pick with extra steps and turns the vote counts on everyone else's screen into decoration. The winner is the head of the repository's existing ordering (vote_count DESC, created_at ASC): most votes, ties to whoever suggested first — deterministic, and re-suggesting a title cannot jump the queue. A room where nobody has voted has no winner and says so, rather than quietly promoting the oldest suggestion as though a vote had happened. host_pick rooms are untouched: the host still promotes freely. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK * fix(watch-together): close the second door into a vote room's selection Gating PromoteSuggestion left SelectItem wide open: it is host-only but was not gated by selection mode, so the host of a vote room could set any title directly and bypass the vote entirely. Enforcing the tally on one path and not the other makes the vote counts on everyone else's screen decoration. A vote room now refuses a direct selection outright. The winner is the only way in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK * fix(watch-together): stop a dropped socket reading as the host leaving hostDisconnectTTL was 15 seconds, which treated any transient drop as a departure. An explicit leave and an explicit close already tear the room down immediately, so this timer only ever covers a host who has NOT said they are going — and at 15s a host who backgrounded the app, moved between screens, or hit a brief network blip lost the room for everyone with a "host_left" nobody could explain. Two minutes survives a reconnect or an app switch, and is short enough that a genuinely departed host does not leave a room open all evening. The janitor still reaps idle rooms independently. This matters for what the clients are growing into: a room you stay in while you browse for something to suggest. A client that drops its socket when the lobby leaves composition should cost you a reconnect, not the room. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK * fix(watch-together): let a vote room actually start its winner The vote gate landed on both doors into a room's selection, but promoting the winner walks through SelectItem to commit — so the gate meant to stop the host bypassing the vote also stopped the vote itself. Vote rooms could not start playback by any route. Split the commit path: SelectItem keeps the gate for direct requests, and PromoteSuggestion goes through the internal path once it has confirmed the suggestion is the winner. Map ErrVoteRoomSelection in the promote handler too, so a future regression there reads as a conflict rather than a 500. Add service-level tests for both gates — the previous tests only covered the pure winnerFrom helper, which is why the suite stayed green while vote rooms were non-functional. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: rxwatcher Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Quick <31828688+Quick104@users.noreply.github.com> --- internal/api/handlers/watch_together.go | 15 ++ internal/watchtogether/service.go | 116 +++++++++++++- internal/watchtogether/vote_selection_test.go | 142 ++++++++++++++++++ internal/watchtogether/vote_winner_test.go | 51 +++++++ 4 files changed, 319 insertions(+), 5 deletions(-) create mode 100644 internal/watchtogether/vote_selection_test.go create mode 100644 internal/watchtogether/vote_winner_test.go 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) + } +}