fix: batch of 22 correctness fixes across server and client (#1371)

* fix(voice): 4 defect(s) (OC-0008, OC-0009, OC-0042, OC-0080)

Guard LiveKit session state against supersession: bump the camera/screen
generation in leaveVoice and teardownForReconnect so an in-flight enable
discards its track, bail out of restoreLocalVoiceState when a newer room
claimed _room mid-await, and recheck isStateConnected in the auto-reconnect
tail.

* fix(ws): 1 defect(s) (OC-0019)

* fix(db): 1 defect(s) (OC-0023)

* fix(ws): 1 defect(s) (OC-0029)

* fix(ws): 1 defect(s) (OC-0032)

* fix(voice): 1 defect(s) (OC-0034)

* fix(admin): 1 defect(s) (OC-0035)

* fix(service): 2 defect(s) (OC-0036, OC-0128)

* fix(voice): 2 defect(s) (OC-0038, OC-0065)

OC-0038: the LiveKit participant_left webhook cleared the leaver's own
client voice state before broadcasting voice_leave, so the broadcast
audience (READ_MESSAGES holders union still-in-the-room participants)
could no longer see them. Voice membership is gated on CONNECT_VOICE
alone, so a participant without READ_MESSAGES never learned the server
had torn down their call. Extracted finishVoiceLeave's audience logic
into broadcastVoiceEventWithLeaver and used it on the webhook path.

OC-0065: handleWebhookParticipantJoined OR'd a GetVoiceState read error
into the same branch as "no matching row", so a transient DB failure
ejected a legitimate participant from the SFU mid-call. Now the read
error is logged and the check skipped, matching sweepStaleVoiceStates.

* fix(client): 1 defect(s) (OC-0041)

* fix(client): 1 defect(s) (OC-0043)

* fix(client): 1 defect(s) (OC-0046)

* fix(client): 1 defect(s) (OC-0047)

* fix(client): 1 defect(s) (OC-0049)

* fix(client): 1 defect(s) (OC-0108)

* fix(client): 2 defect(s) (OC-0111, OC-0143)

OC-0111: retry a presence_update dropped by the 1-per-10s limiter once the
window reopens, so auto-idle's return-to-online does not leave the server
and every other client stuck on idle.

OC-0143: pass apiConfig.host to the DM profile sidebar so per-user notes
are scoped per server, matching channel mutes, the NSFW gate and volume.

* test(ws): align aborted-switch test with OC-0034 no-resurrect behavior

The fix agent rewrote this pre-existing test (it locked the buggy restore
path) but the prove agent left it out of c67d25ed; committed state alone
failed go test ./ws/ without it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
J3vb
2026-08-14 16:15:32 +02:00
committed by GitHub
co-authored by Claude Fable 5
parent 8579cb5d91
commit 7be9ccd2f9
41 changed files with 1569 additions and 89 deletions
+37
View File
@@ -709,6 +709,43 @@ func TestAdminAPI_DeleteChannel_CleansVoiceBeforeDBDelete(t *testing.T) {
}
}
// A voice_join racing the delete window must be refused, not silently create
// a voice_states row the FK cascade then wipes out from under it (OC-0035):
// CleanupVoiceForChannel snapshots participants ONCE, up front, so a join
// that lands after that snapshot but before AdminDeleteChannel's cascade
// leaves the joiner's hub-side voice state and LiveKit session orphaned with
// nothing left to clean it up. handleDeleteChannel must close that window the
// same way the archive path does (handlePatchChannel): persist archived=true
// BEFORE evicting current participants, so voice_join's archived gate
// (ws/voice_join.go) refuses any concurrent join that reads the channel row
// during cleanup.
func TestAdminAPI_DeleteChannel_ArchivesBeforeVoiceCleanup(t *testing.T) {
database := openAdminTestDB(t)
hub := &mockHub{}
handler := admin.NewAdminAPI(database, "1.0.0", hub, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
token := createAdminUser(t, database)
chID, _ := database.AdminCreateChannel(context.Background(), "del-race", "voice", "", "", 0)
archivedAtCleanup := false
hub.onVoiceCleanup = func(channelID int64) {
ch, err := database.GetChannel(context.Background(), channelID)
if err != nil || ch == nil {
t.Fatalf("GetChannel during cleanup: %v", err)
}
archivedAtCleanup = ch.Archived
}
w := doRequest(t, handler, http.MethodDelete, "/channels/"+itoa(chID), token, nil)
if w.Code != http.StatusNoContent {
t.Fatalf("status = %d, want 204; body: %s", w.Code, w.Body.String())
}
if !archivedAtCleanup {
t.Errorf("channel.Archived at CleanupVoiceForChannel time = false, want true — a concurrent voice_join would not be refused by the archived gate")
}
}
func TestAdminAPI_DeleteChannel_NotFound(t *testing.T) {
database := openAdminTestDB(t)
handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
+27
View File
@@ -281,6 +281,33 @@ func handleDeleteChannel(database *db.DB, hub HubBroadcaster) http.HandlerFunc {
}
id := existing.ID
// Mark the channel archived BEFORE evicting participants, mirroring the
// archive path (handlePatchChannel): CleanupVoiceForChannel snapshots
// voice participants ONCE, up front, so a voice_join racing this delete
// could otherwise read the still-live channel row, pass the archived
// gate (ws/voice_join.go), and insert a voice_states row after the
// snapshot but before AdminDeleteChannel's cascade — leaving that
// joiner's hub-side voice state and LiveKit session orphaned with no
// DB row left for any sweep to find (OC-0035). Persisting archived=1
// first makes voice_join's existing archived check refuse that join
// outright, the same way it already refuses one racing an archive.
if !existing.Archived {
if err := database.AdminUpdateChannel(r.Context(), id, db.ChannelUpdate{
Name: existing.Name,
Topic: existing.Topic,
Category: existing.Category,
SlowMode: existing.SlowMode,
Position: existing.Position,
Archived: true,
NSFW: existing.NSFW,
VoiceMaxUsers: existing.VoiceMaxUsers,
VoiceMaxVideo: existing.VoiceMaxVideo,
}); err != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to delete channel")
return
}
}
// Evict voice participants BEFORE deleting the row: the voice_states
// FK cascade wipes the rows the cleanup reads, and the stale sweeper
// cannot recover participants of a channel that no longer exists.
+35
View File
@@ -774,6 +774,41 @@ func TestListMembers_ExcludesBanned(t *testing.T) {
}
}
// TestListMembers_LapsedTempBan_StillIncluded locks the same "reconverged raw
// column" fix that GetUserIDsByUsernames and ListMentionTargetsByRoles already
// carry (db/mention_queries.go's notBannedClause): nothing clears users.banned
// when a temp ban's ban_expires lapses (that's decided lazily, at login, by
// auth.IsEffectivelyBanned), so a raw `banned = 0` filter leaves a reinstated
// user permanently absent from the member roster even though they can log in
// and post again.
func TestListMembers_LapsedTempBan_StillIncluded(t *testing.T) {
database := newTestDB(t)
_, _ = database.CreateUser(context.Background(), "member_visible", "hash", 4)
id2, _ := database.CreateUser(context.Background(), "member_lapsed_ban", "hash", 4)
past := time.Now().Add(-1 * time.Hour)
if err := database.BanUser(context.Background(), id2, "temp ban", &past); err != nil {
t.Fatalf("BanUser: %v", err)
}
members, err := database.ListMembers(context.Background())
if err != nil {
t.Fatalf("ListMembers: %v", err)
}
found := false
for _, m := range members {
if m.Username == "member_lapsed_ban" {
found = true
}
}
if !found {
t.Error("a lapsed temp ban must not hide the user from the member roster")
}
if len(members) != 2 {
t.Errorf("ListMembers() = %d, want 2 (lapsed ban must not hide the member)", len(members))
}
}
func TestListMembers_SortedByUsername(t *testing.T) {
database := newTestDB(t)
_, _ = database.CreateUser(context.Background(), "zeta_user", "hash", 4)
+1 -1
View File
@@ -128,7 +128,7 @@ SELECT u.id, u.username, u.avatar, u.status, LOWER(r.name), u.identity_public_ke
u.display_name, u.custom_status
FROM users u
JOIN roles r ON u.role_id = r.id
WHERE u.banned = 0
WHERE (u.banned = 0 OR (u.ban_expires IS NOT NULL AND replace(u.ban_expires, ' ', 'T') <= strftime('%Y-%m-%dT%H:%M:%SZ', 'now')))
ORDER BY u.username ASC
`
+1 -1
View File
@@ -55,7 +55,7 @@ SELECT u.id, u.username, u.avatar, u.status, LOWER(r.name), u.identity_public_ke
u.display_name, u.custom_status
FROM users u
JOIN roles r ON u.role_id = r.id
WHERE u.banned = 0
WHERE (u.banned = 0 OR (u.ban_expires IS NOT NULL AND replace(u.ban_expires, ' ', 'T') <= strftime('%Y-%m-%dT%H:%M:%SZ', 'now')))
ORDER BY u.username ASC;
-- name: CountUsers :one
+18 -9
View File
@@ -52,14 +52,6 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
return nil, err
}
// Slow mode (non-DM only).
if !isDM && ch.SlowMode > 0 && !s.perms.HasChannelPerm(ctx, p.UserID, p.ChannelID, permissions.ManageMessages) {
slowKey := auth.Key(auth.Key("slow", p.UserID), p.ChannelID)
if s.limiter != nil && !s.limiter.Allow(slowKey, 1, time.Duration(ch.SlowMode)*time.Second) {
return nil, fmt.Errorf("%w: channel has %ds slow mode", ErrSlowMode, ch.SlowMode)
}
}
// Validate and sanitize content.
content, err := sanitizeContent(p.Content, len(p.AttachmentIDs) > 0)
if err != nil {
@@ -73,6 +65,19 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
}
}
// Slow mode (non-DM only). Deliberately checked last, after content and
// attachment validation: Allow() below records the cooldown timestamp the
// instant it returns true, so a send that fails validation after this
// point must not have already spent the once-per-window token — that
// would lock the composer for up to ch.SlowMode seconds for a send that
// never actually posted anything.
if !isDM && ch.SlowMode > 0 && !s.perms.HasChannelPerm(ctx, p.UserID, p.ChannelID, permissions.ManageMessages) {
slowKey := auth.Key(auth.Key("slow", p.UserID), p.ChannelID)
if s.limiter != nil && !s.limiter.Allow(slowKey, 1, time.Duration(ch.SlowMode)*time.Second) {
return nil, fmt.Errorf("%w: channel has %ds slow mode", ErrSlowMode, ch.SlowMode)
}
}
// Resolve mentions against the sanitized content, before the insert, so the
// row and its mention set are written together. Unknown @words and an
// unauthorized @everyone resolve to nothing and stay plain text.
@@ -122,7 +127,11 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
return nil, fmt.Errorf("%w: message content cannot be empty", ErrBadRequest)
}
if linked > 0 {
attMap, attErr := s.st.GetAttachmentsByMessageIDs(ctx, []int64{msgID})
// Detached from ctx for the same reason as the compensating deletes
// above: the link already committed, so a request ctx canceled the
// instant it returns (sender disconnects right after) must not turn
// a successful attachment-only send into a blank broadcast bubble.
attMap, attErr := s.st.GetAttachmentsByMessageIDs(context.WithoutCancel(ctx), []int64{msgID})
if attErr != nil {
slog.Error("MessageService.SendMessage GetAttachments", "err", attErr)
} else {
+96
View File
@@ -9,8 +9,10 @@ package service
import (
"context"
"errors"
"strings"
"testing"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
"github.com/owncord/server/permissions"
)
@@ -306,3 +308,97 @@ func TestDeleteMessage_FailsClosedWhenChannelLookupErrors(t *testing.T) {
t.Fatalf("message must survive the refused delete; GetMessage: msg=%v err=%v", msg, err)
}
}
// OC-0036: slow mode's cooldown token is spent by limiter.Allow, which must
// only run once the send has passed content/attachment validation. Consuming
// it earlier means a send that gets rejected for an unrelated reason (content
// too long, in this case) still locks the composer for the full slow-mode
// window even though nothing was ever posted.
func TestSendMessage_SlowModeNotConsumedByFailedContentValidation(t *testing.T) {
_, database := newTestMessageService(t)
if err := database.SetChannelSlowMode(context.Background(), 10, 3600); err != nil {
t.Fatalf("SetChannelSlowMode: %v", err)
}
checker := permissions.NewChecker(database)
permSvc := NewPermissionService(database, checker)
svc := NewMessageService(database, permSvc, auth.NewRateLimiter())
ctx := context.Background()
overLong := strings.Repeat("a", maxMessageLen+1)
if _, err := svc.SendMessage(ctx, SendMessageParams{
ChannelID: 10, UserID: 1, Username: "alice", Content: overLong,
}); !errors.Is(err, ErrBadRequest) {
t.Fatalf("over-length send: err = %v, want ErrBadRequest", err)
}
// The rejected send above must not have spent the once-per-hour slow-mode
// token: a valid, short send immediately after should still go through.
if _, err := svc.SendMessage(ctx, SendMessageParams{
ChannelID: 10, UserID: 1, Username: "alice", Content: "hi",
}); err != nil {
t.Fatalf("SendMessage right after a rejected over-length send: %v — slow mode must only be "+
"charged once a send clears content/attachment validation, not before", err)
}
}
// disconnectAfterLinkStore models a client whose connection drops the instant
// LinkAttachmentsToMessage commits — mirrors disconnectAfterWriteStore but for
// the attachment path. GetAttachmentsByMessageIDs is overridden to fail
// whenever handed an already-canceled context, so a test can tell whether the
// post-link attachment read used the (canceled) request ctx or a detached one.
type disconnectAfterLinkStore struct {
Store
cancel context.CancelFunc
}
func (s disconnectAfterLinkStore) LinkAttachmentsToMessage(ctx context.Context, messageID, uploaderID int64, attachmentIDs []string) (int64, error) {
n, err := s.Store.LinkAttachmentsToMessage(ctx, messageID, uploaderID, attachmentIDs)
s.cancel()
return n, err
}
func (s disconnectAfterLinkStore) GetAttachmentsByMessageIDs(ctx context.Context, msgIDs []int64) (map[int64][]db.AttachmentInfo, error) {
if ctx.Err() != nil {
return nil, ctx.Err()
}
return s.Store.GetAttachmentsByMessageIDs(ctx, msgIDs)
}
// OC-0128: a sender whose connection drops the instant the attachment link
// commits must still get the linked attachment back on the broadcast result —
// not a message with no content and no attachments. The post-link read must
// run on a detached ctx, the same way the compensating deletes in SendMessage
// already do.
func TestSendMessage_AttachmentsSurviveSenderDisconnectAfterLink(t *testing.T) {
_, database := newTestMessageService(t)
// Grant ATTACH_FILES on top of the base member perms newTestMessageService seeds.
seedRole(t, database, &db.Role{
ID: permissions.MemberRoleID,
Name: "member",
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions | permissions.AttachFiles,
Position: 1,
})
if _, err := database.ExecContext(context.Background(),
`INSERT INTO attachments (id, uploader_id, filename, stored_as, mime_type, size)
VALUES (?, ?, ?, ?, ?, ?)`,
"att-1", 1, "photo.png", "stored-photo.png", "image/png", 100,
); err != nil {
t.Fatalf("seed attachment: %v", err)
}
checker := permissions.NewChecker(database)
permSvc := NewPermissionService(database, checker)
ctx, cancel := context.WithCancel(context.Background())
svc := NewMessageService(disconnectAfterLinkStore{Store: database, cancel: cancel}, permSvc, nil)
result, err := svc.SendMessage(ctx, SendMessageParams{
ChannelID: 10, UserID: 1, Username: "alice", AttachmentIDs: []string{"att-1"},
})
if err != nil {
t.Fatalf("SendMessage: %v", err)
}
if len(result.Attachments) != 1 {
t.Fatalf("Attachments = %v, want 1 — a disconnect right after the attachment link commits must not "+
"broadcast a blank message bubble", result.Attachments)
}
}
+19 -11
View File
@@ -108,11 +108,19 @@ func TestSweepStaleVoiceStates_EvictionIsScopedToCheckedChannel(t *testing.T) {
}
}
// When a voice channel switch aborts because the old row's delete failed,
// the abort branch restores the in-memory voice state — and must restore the
// voice-topic subscription and key-holder entry torn down with it, or the
// client silently misses every voice_e2ee relay for the session it is still in.
func TestHandleVoiceJoin_AbortedSwitchRestoresVoiceTopicSubscription(t *testing.T) {
// When a voice channel switch aborts because the old row's delete failed, the
// abort branch used to restore the in-memory voice state, voice-topic
// subscription and key-holder entry torn down by the leave that preceded it.
// OC-0034: that restore was itself the bug. handleVoiceLeave's
// finishVoiceLeave always broadcasts voice_leave for the old channel to the
// leaver themselves (voice_leave.go), and the client tears its own session
// down on a self voice_leave — so by the time the abort branch runs, every
// client including this user's own has already forgotten the old membership.
// Restoring the server's view of it resurrects a session nobody else
// believes exists, with no re-broadcast to tell them otherwise. The fix
// leaves the client's voice state cleared on abort so it agrees with the
// voice_leave already sent; the periodic sweep reaps the orphaned DB row.
func TestHandleVoiceJoin_AbortedSwitchDoesNotResurrectVoiceTopicSubscription(t *testing.T) {
ctx := context.Background()
database := newHarvestVoiceDB(t)
uid := seedHarvestVoiceUser(t, database, "abort-switch")
@@ -160,14 +168,14 @@ func TestHandleVoiceJoin_AbortedSwitchRestoresVoiceTopicSubscription(t *testing.
h.handleVoiceJoin(ctx, c, json.RawMessage(fmt.Sprintf(`{"channel_id": %d}`, chB)))
if got := c.getVoiceChID(); got != chA {
t.Fatalf("aborted switch left client voice state at %d, want restored channel %d", got, chA)
if got := c.getVoiceChID(); got != 0 {
t.Fatalf("aborted switch resurrected client voice state at channel %d, want 0 — voice_leave for channel %d was already broadcast to this client (OC-0034)", got, chA)
}
if !h.SubscribedToVoiceTopicForTest(c, chA) {
t.Error("aborted switch did not re-subscribe the client to its channel's voice topic — every voice_e2ee relay for the restored session is silently dropped")
if h.SubscribedToVoiceTopicForTest(c, chA) {
t.Error("aborted switch re-subscribed the client to a voice topic for a channel it already received voice_leave for")
}
if !h.IsVoiceKeyHolder(chA, uid) {
t.Error("aborted switch left the key-holder map without the channel's only participant")
if h.IsVoiceKeyHolder(chA, uid) {
t.Error("aborted switch left the client named as key holder for a channel it already left")
}
}
+17
View File
@@ -545,6 +545,23 @@ func (h *Hub) unregisterNow(c *Client) bool {
return exists
}
// shouldMarkOffline reports whether a disconnect teardown should run
// MarkUserDisconnected and broadcast an offline presence for c's user.
//
// `replaced` (unregisterNow's return, sampled once at the start of teardown)
// is necessary but not sufficient: both readPump's defer and
// unregisterFailedHandshake sample it BEFORE handleVoiceLeave, which can
// block for seconds (DB delete, audience scan, a LiveKit call bounded by
// lkTimeout=5s). A reconnect landing during that window registers a new
// client for the same user and is invisible to the stale boolean, so the
// dead connection's teardown would otherwise mark the live session offline
// (OC-0019). Re-checking h.clients at decision time closes that gap: any
// entry present once c has been removed is necessarily a newer connection —
// unregisterNow only ever deletes c's own slot, never someone else's.
func (h *Hub) shouldMarkOffline(c *Client, replaced bool) bool {
return !replaced && h.GetClient(c.userID) == nil
}
// ClientCount returns the number of currently registered clients (test helper).
func (h *Hub) ClientCount() int {
h.mu.RLock()
+29
View File
@@ -79,6 +79,35 @@ func (h *Hub) broadcastVoiceEvent(ctx context.Context, channelID int64, msg []by
h.broadcastChannelScopedTo(channelID, msg, audience, "voice event")
}
// broadcastVoiceEventWithLeaver is broadcastVoiceEvent extended to guarantee
// leaverID is in the audience even though the caller has already cleared
// their client-side voice state — which means broadcastVoiceEvent's own
// still-in-the-room participant union can no longer see them. Every path
// that tears down a voice participant whose client state is cleared before
// the voice_leave goes out needs this: voice membership is gated on
// CONNECT_VOICE alone, so a leaver without READ_MESSAGES on the channel
// would otherwise never learn the server already ended their call. Mirrors
// CleanupVoiceForChannel's per-batch leaver union, for the single-leaver case.
func (h *Hub) broadcastVoiceEventWithLeaver(ctx context.Context, channelID int64, msg []byte, leaverID int64) {
audience := h.channelReadAudience(ctx, channelID)
seen := make(map[int64]struct{}, len(audience)+1)
for _, uid := range audience {
seen[uid] = struct{}{}
}
h.mu.RLock()
for uid, c := range h.clients {
if _, ok := seen[uid]; !ok && c.getVoiceChID() == channelID {
seen[uid] = struct{}{}
audience = append(audience, uid)
}
}
h.mu.RUnlock()
if _, ok := seen[leaverID]; !ok {
audience = append(audience, leaverID)
}
h.broadcastChannelScopedTo(channelID, msg, audience, "voice event")
}
// broadcastChannelScoped enqueues msg for exactly the connected clients whose
// current role may READ channelID, tagged with that channel id so reconnect
// replay filters it too (EventsSinceFiltered replays a channelID of 0
+50
View File
@@ -13,6 +13,7 @@ import (
"github.com/go-chi/chi/v5"
"github.com/owncord/server/config"
"github.com/owncord/server/permissions"
"github.com/owncord/server/ws"
)
@@ -565,6 +566,55 @@ func TestWebhook_ParticipantLeft_ClearsE2EEState_OnMatch(t *testing.T) {
}
}
// TestWebhook_ParticipantLeft_LeaverWithoutReadStillNotified locks OC-0038:
// voice membership is gated on CONNECT_VOICE alone, so a participant can be
// in a voice channel without READ_MESSAGES on it. The webhook-driven teardown
// clears the leaver's own client voice state before broadcasting, so
// broadcastVoiceEvent's audience — (READ_MESSAGES holders) (still-in-the-
// room participants) — can no longer see them, and they never learn the
// server already tore down their call. finishVoiceLeave and
// CleanupVoiceForChannel both add the leaver to the audience for exactly
// this reason; the webhook path must too.
func TestWebhook_ParticipantLeft_LeaverWithoutReadStillNotified(t *testing.T) {
t.Parallel()
hub, database := newVoiceHub(t)
chanID := seedVoiceChannel(t, database, "webhook-noread-ch")
// Role 3 (Moderator) carries CONNECT_VOICE in its default mask but lacks
// the Administrator bit, so a channel-scoped READ_MESSAGES deny actually
// applies — an Owner/Admin role would bypass channel_overrides entirely.
if _, err := database.CreateUser(context.Background(), "webhook-noread-user", "hash", 3); err != nil {
t.Fatalf("CreateUser: %v", err)
}
leaver, err := database.GetUserByUsername(context.Background(), "webhook-noread-user")
if err != nil || leaver == nil {
t.Fatalf("GetUserByUsername: %v", err)
}
if err := database.UpsertChannelOverride(context.Background(), chanID, 3, 0, permissions.ReadMessages); err != nil {
t.Fatalf("UpsertChannelOverride: %v", err)
}
if err := database.JoinVoiceChannel(context.Background(), leaver.ID, chanID); err != nil {
t.Fatalf("JoinVoiceChannel: %v", err)
}
vs, err := database.GetVoiceState(context.Background(), leaver.ID)
if err != nil || vs == nil {
t.Fatalf("GetVoiceState: %v (nil=%v)", err, vs == nil)
}
leaverSend := make(chan []byte, 16)
c := ws.NewTestClient(hub, leaver.ID, leaverSend)
ws.SetClientVoiceStateForTest(c, chanID, vs.JoinedAt)
hub.RegisterNowForTest(c)
hub.HandleWebhookParticipantLeftForTest(leaver.ID, chanID, vs.JoinedAt)
if got := countVoiceLeaves(leaverSend, 200*time.Millisecond); got == 0 {
t.Error("the leaver's own client, denied READ_MESSAGES on the voice channel, received no voice_leave after the LiveKit webhook tore down its own session")
}
}
// ---------------------------------------------------------------------------
// livekit_process.go generateConfig tests
// ---------------------------------------------------------------------------
+20 -2
View File
@@ -129,7 +129,18 @@ func (h *Hub) handleWebhookParticipantJoined(ctx context.Context, event *livekit
// so we remove the rogue participant from LiveKit.
if h.db != nil {
state, stateErr := h.db.GetVoiceState(ctx, userID)
if stateErr != nil || state == nil || state.ChannelID != channelID {
if stateErr != nil {
// A transient read failure (I/O error, lock contention, a
// maintenance window) is not proof of a rogue participant —
// treating it as one would eject a legitimate participant from
// the SFU on a single bad read. Mirrors sweepStaleVoiceStates'
// hasChannelPermChecked guard: skip and let the participant be;
// a later webhook retry or sweep tick resolves it.
slog.Error("livekit webhook: GetVoiceState failed, skipping rogue-participant check",
"error", stateErr, "user_id", userID, "channel_id", channelID)
return
}
if state == nil || state.ChannelID != channelID {
slog.Warn("livekit webhook: rogue participant_joined — no matching voice state, removing",
"user_id", userID, "channel_id", channelID)
if h.livekit != nil {
@@ -226,7 +237,14 @@ func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.W
// rejected with NOT_KEY_HOLDER. Safe here: no locks are held.
h.updateKeyHolder(channelID)
h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, userID))
// The leaver's own client state was just cleared above, so
// broadcastVoiceEvent's still-in-the-room union can no longer see
// them — without broadcastVoiceEventWithLeaver's extra term, a
// participant without READ_MESSAGES on this channel (voice
// membership needs only CONNECT_VOICE) never learns the server
// already tore down their call. Mirrors finishVoiceLeave and
// CleanupVoiceForChannel, which add the leaver for the same reason.
h.broadcastVoiceEventWithLeaver(ctx, channelID, buildVoiceLeave(channelID, userID), userID)
slog.Info("livekit webhook: cleaned up stale voice state",
"user_id", userID,
"channel_id", channelID)
+37
View File
@@ -104,6 +104,43 @@ func TestWebhook_ParticipantJoined_ValidJoinAccepted(t *testing.T) {
}
}
// TestWebhook_ParticipantJoined_TransientReadErrorDoesNotEvict locks OC-0065:
// a GetVoiceState read failure must not be treated as proof of a rogue
// participant. sweepStaleVoiceStates already draws this distinction via
// hasChannelPermChecked ("a transient read failure ... is not a revocation");
// the webhook path OR'd stateErr into the same branch as "no matching row",
// so a transient DB error (SQLITE_BUSY, an I/O blip) ejected a legitimate
// participant from the SFU mid-call.
func TestWebhook_ParticipantJoined_TransientReadErrorDoesNotEvict(t *testing.T) {
hub, database := newVoiceHub(t)
user := seedVoiceOwner(t, database, "joined-dberr-user")
chanID := seedVoiceChan(t, database, "joined-dberr-ch")
if err := database.JoinVoiceChannel(context.Background(), user.ID, chanID); err != nil {
t.Fatalf("JoinVoiceChannel: %v", err)
}
// Fault-inject exactly the GetVoiceState read: renaming the table out from
// under the query makes it return a genuine DB error instead of the
// sql.ErrNoRows GetVoiceState collapses to (nil, nil) for a real "no
// membership" case.
if _, err := database.ExecContext(context.Background(),
`ALTER TABLE voice_states RENAME TO voice_states_offline`); err != nil {
t.Fatalf("rename voice_states: %v", err)
}
logs := captureLogs(t)
hub.HandleWebhookParticipantJoinedForTest(
participantIdentityFor(user.ID, "some-token"),
roomNameFor(chanID),
)
if out := logs(); strings.Contains(out, "rogue participant_joined") {
t.Errorf("a transient GetVoiceState error was treated as a rogue participant and evicted; log:\n%s", out)
}
}
func TestWebhook_ParticipantJoined_WrongChannelFlagged(t *testing.T) {
hub, database := newVoiceHub(t)
user := seedVoiceOwner(t, database, "joined-wrongch-user")
+8
View File
@@ -452,6 +452,14 @@ func (h *Hub) unregisterFailedHandshake(ctx context.Context, c *Client) {
if voiceChID != 0 {
h.handleVoiceLeave(cleanupCtx, c)
}
}
// shouldMarkOffline re-checks h.clients rather than trusting the
// `replaced` snapshot alone: it was sampled before handleVoiceLeave,
// which can block for seconds, so a reconnect landing during that window
// would otherwise be invisible here and mark the live session's user
// offline (OC-0019, mirrored from readPump's defer in serve_pumps.go).
if h.shouldMarkOffline(c, replaced) {
cleanupCtx := context.WithoutCancel(ctx)
_ = h.db.MarkUserDisconnected(cleanupCtx, c.userID)
// custom_status is nil, not c.user.CustomStatus: see the identical
// note in serve_pumps.go's readPump defer — that field is an
+7 -1
View File
@@ -183,7 +183,13 @@ func readPump(ctx context.Context, conn *websocket.Conn, hub *Hub, c *Client) {
}
slog.Info("websocket disconnected", attrs...)
if !replaced {
// shouldMarkOffline re-checks h.clients instead of trusting
// `replaced` alone: that flag was sampled before handleVoiceLeave,
// which can block for seconds, so a reconnect landing during that
// window would otherwise be invisible here and this dead
// connection's teardown would mark the live session's user
// offline (OC-0019).
if hub.shouldMarkOffline(c, replaced) {
// A real disconnect is offline for everyone, the user
// included, so this path needs no invisible mapping. The row,
// however, keeps a *chosen* status (idle/dnd/invisible)
@@ -0,0 +1,165 @@
package ws
// serve_pumps_reconnect_race_test.go — regression test for OC-0019.
//
// readPump's defer snapshots `replaced := hub.unregisterNow(c)` BEFORE running
// hub.handleVoiceLeave, which can block for seconds (DB delete, audience scan,
// a LiveKit RemoveParticipant HTTP call bounded by lkTimeout=5s). The stale
// `replaced` boolean is then reused, unchecked, to decide whether to run
// MarkUserDisconnected and broadcast an offline presence. A reconnect that
// registers during that window is invisible to the stale flag: the dead
// socket's teardown marks the *live* session's user offline.
import (
"bytes"
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/coder/websocket"
lkproto "github.com/livekit/protocol/livekit"
"google.golang.org/protobuf/proto"
"github.com/owncord/server/auth"
"github.com/owncord/server/config"
"github.com/owncord/server/db"
)
// TestReadPump_ReconnectDuringVoiceCleanup_DoesNotMarkUserOffline reproduces
// the finding's repro: a client's socket drops while it holds a voice
// session, its readPump defer starts tearing down (unregisterNow already
// removed it from the hub), and — while handleVoiceLeave is still blocked on
// the LiveKit call — the same user reconnects and takes the hub slot. The
// defer must not go on to mark that user offline once it resumes.
func TestReadPump_ReconnectDuringVoiceCleanup_DoesNotMarkUserOffline(t *testing.T) {
database := newHarvestVoiceDB(t)
uid := seedHarvestVoiceUser(t, database, "reconnect-race")
chID := mustCreateVoiceChannel(t, database, "voice-race")
ctx := context.Background()
if err := database.JoinVoiceChannel(ctx, uid, chID); err != nil {
t.Fatalf("JoinVoiceChannel: %v", err)
}
if err := database.UpdateUserStatus(ctx, uid, "online"); err != nil {
t.Fatalf("UpdateUserStatus: %v", err)
}
// Fake LiveKit server: holds the RemoveParticipant response until the
// test releases it, giving full control over handleVoiceLeave's window.
reachedLiveKit := make(chan struct{})
proceed := make(chan struct{})
lkSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
close(reachedLiveKit)
<-proceed
body, _ := proto.Marshal(&lkproto.RemoveParticipantResponse{})
w.Header().Set("Content-Type", "application/protobuf")
w.WriteHeader(http.StatusOK)
_, _ = w.Write(body)
}))
defer lkSrv.Close()
lk, err := NewLiveKitClient(&config.VoiceConfig{
LiveKitAPIKey: "testkeytestkeytest",
LiveKitAPISecret: "testsecrettestsecrettestsecret",
LiveKitURL: "ws://" + lkSrv.Listener.Addr().String(),
})
if err != nil {
t.Fatalf("NewLiveKitClient: %v", err)
}
h := NewHub(database, auth.NewRateLimiter(), nil)
h.SetLiveKit(lk)
c := NewTestClient(h, uid, make(chan []byte, 8))
c.user = &db.User{ID: uid, Status: "online"}
c.setVoiceState(chID, "tok-race")
h.clients[uid] = c
// Real server-side *websocket.Conn, closed immediately so readPump's
// first Read fails and its defer runs — mirrors
// serve_reconnect_double_teardown_test.go's setup.
connCh := make(chan *websocket.Conn, 1)
wsSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
conn, acceptErr := websocket.Accept(w, r, nil)
if acceptErr != nil {
return
}
_ = conn.CloseNow()
connCh <- conn
}))
defer wsSrv.Close()
dialCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
clientConn, resp, dialErr := websocket.Dial(dialCtx, "ws"+strings.TrimPrefix(wsSrv.URL, "http"), nil)
if resp != nil && resp.Body != nil {
_ = resp.Body.Close()
}
if dialErr != nil {
t.Fatalf("dial: %v", dialErr)
}
defer func() { _ = clientConn.Close(websocket.StatusNormalClosure, "") }()
var conn *websocket.Conn
select {
case conn = <-connCh:
case <-time.After(5 * time.Second):
t.Fatal("server never accepted the connection")
}
done := make(chan struct{})
go func() {
readPump(context.Background(), conn, h, c)
close(done)
}()
// Wait until the defer is blocked inside handleVoiceLeave's LiveKit call —
// unregisterNow has already run and sampled replaced=false.
select {
case <-reachedLiveKit:
case <-time.After(5 * time.Second):
t.Fatal("readPump's defer never reached the LiveKit RemoveParticipant call")
}
// The user reconnects while the old connection's teardown is still in
// flight: a fresh client takes the (now-empty) hub slot for the same
// user, exactly as registerNow does for a real reconnect.
newClient := NewTestClient(h, uid, make(chan []byte, 8))
newClient.user = &db.User{ID: uid, Status: "online"}
h.registerNow(newClient, map[int64]bool{})
// Let handleVoiceLeave's LiveKit call complete so the old defer resumes.
close(proceed)
select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("readPump did not return after the LiveKit call completed")
}
if got := h.GetClient(uid); got != newClient {
t.Fatalf("hub client for user %d = %p after old connection's teardown, want the reconnected client %p", uid, got, newClient)
}
var offlineBroadcasts int
for len(h.broadcast) > 0 {
bm := <-h.broadcast
if bytes.Contains(bm.msg, []byte(`"status":"offline"`)) {
offlineBroadcasts++
}
}
if offlineBroadcasts != 0 {
t.Errorf("got %d offline presence broadcasts after a reconnect raced the old connection's voice cleanup, want 0 — the live session was stamped offline", offlineBroadcasts)
}
user, err := database.GetUserByID(ctx, uid)
if err != nil {
t.Fatalf("GetUserByID: %v", err)
}
if user.Status != "online" {
t.Errorf("user status = %q after the reconnect race, want %q — the dead socket's teardown overwrote the live session's status", user.Status, "online")
}
}
+3 -6
View File
@@ -152,8 +152,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol
members, err := database.ListMembers(ctx)
if err != nil {
slog.Warn("buildReady ListMembers", "err", err)
members = []db.MemberSummary{}
return nil, fmt.Errorf("buildReady ListMembers: %w", err)
}
members = h.presentableMembers(members, userID)
@@ -187,8 +186,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol
// Per-user unread counts.
unreadMap, err := database.GetChannelUnreadCounts(ctx, userID)
if err != nil {
slog.Warn("buildReady GetChannelUnreadCounts", "err", err)
unreadMap = map[int64]db.ChannelUnread{}
return nil, fmt.Errorf("buildReady GetChannelUnreadCounts: %w", err)
}
// Build protocol-compliant channel objects (strip extra fields).
@@ -241,8 +239,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol
// this a DM voice call's voice_state rows would never make it into ready.
dmChannels, err := database.GetUserDMChannels(ctx, userID)
if err != nil {
slog.Warn("buildReady GetUserDMChannels", "err", err)
dmChannels = []db.DMChannelInfo{}
return nil, fmt.Errorf("buildReady GetUserDMChannels: %w", err)
}
// GetUserDMChannels computes unread from read_states but carries no mention
// count, so a DM mention badge used to vanish on every reconnect. The
@@ -0,0 +1,83 @@
package ws_test
// serve_ready_error_propagation_test.go — regression test for finding
// OC-0029: buildReady downgraded ListMembers, GetChannelUnreadCounts, and
// GetUserDMChannels failures to a slog.Warn plus an empty value, then still
// built and returned a normal `ready` frame. `ready` is the protocol's
// authoritative full-state snapshot -- dispatcher.ts treats an empty
// dm_channels as "the server always sends this field, so empty means no open
// DMs" and wipes dmStore (and the active channel, if a DM was open) on that
// basis -- so a transient DB error on any of these three queries was
// indistinguishable on the wire from "you genuinely have none".
// ListChannels/ListRoles/GetChannelOverridesFor already do the right thing
// (return the error and abort the handshake so the client retries); these
// three should too.
//
// Each subtest fault-injects exactly one of the three queries by dropping
// the SQLite table only that query (and nothing earlier in buildReady's call
// order) depends on, then asserts buildReady fails instead of shipping a
// falsely-empty snapshot.
import (
"context"
"testing"
)
// TestBuildReady_PropagatesListMembersError drops `users`, which ListMembers
// joins against but which nothing earlier in buildReady (ListChannels,
// ListRoles) touches.
func TestBuildReady_PropagatesListMembersError(t *testing.T) {
hub, database := newServeHub(t)
user := seedServeUser(t, database, "ready-err-members")
role, err := database.GetRoleByID(context.Background(), user.RoleID)
if err != nil || role == nil {
t.Fatalf("GetRoleByID: %v", err)
}
if _, err := database.ExecContext(context.Background(), `DROP TABLE users`); err != nil {
t.Fatalf("drop users: %v", err)
}
if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil {
t.Fatal("buildReady must fail the handshake when ListMembers errors, not silently ship an empty member list as if the server genuinely has none")
}
}
// TestBuildReady_PropagatesUnreadCountsError drops `read_states`, which only
// GetChannelUnreadCounts (and, further down the function, GetUserDMChannels)
// reads -- ListChannels, ListRoles and ListMembers do not.
func TestBuildReady_PropagatesUnreadCountsError(t *testing.T) {
hub, database := newServeHub(t)
user := seedServeUser(t, database, "ready-err-unread")
role, err := database.GetRoleByID(context.Background(), user.RoleID)
if err != nil || role == nil {
t.Fatalf("GetRoleByID: %v", err)
}
if _, err := database.ExecContext(context.Background(), `DROP TABLE read_states`); err != nil {
t.Fatalf("drop read_states: %v", err)
}
if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil {
t.Fatal("buildReady must fail the handshake when GetChannelUnreadCounts errors, not silently ship every channel with unread_count/mention_count zeroed")
}
}
// TestBuildReady_PropagatesDMChannelsError drops `dm_open_state`, which only
// GetUserDMChannels reads -- nothing else in buildReady's call chain does.
func TestBuildReady_PropagatesDMChannelsError(t *testing.T) {
hub, database := newServeHub(t)
user := seedServeUser(t, database, "ready-err-dms")
role, err := database.GetRoleByID(context.Background(), user.RoleID)
if err != nil || role == nil {
t.Fatalf("GetRoleByID: %v", err)
}
if _, err := database.ExecContext(context.Background(), `DROP TABLE dm_open_state`); err != nil {
t.Fatalf("drop dm_open_state: %v", err)
}
if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil {
t.Fatal("buildReady must fail the handshake when GetUserDMChannels errors, not silently ship dm_channels: [] as if the user genuinely has none")
}
}
+61
View File
@@ -1207,6 +1207,67 @@ func TestVoice_Join_SameChannel_IsIdempotent(t *testing.T) {
}
}
// TestVoice_Join_AbortedSwitch_DoesNotResurrectPhantomSession pins OC-0034:
// when a voice-channel switch's pre-switch leave leaves the old voice_states
// row in place (e.g. a missing join token short-circuits the delete via
// leaveVoiceChannelWithRetry's empty-token guard in voice_leave.go),
// handleVoiceJoin aborts the switch. finishVoiceLeave has already broadcast
// voice_leave for the old channel to every client that can see it — including
// the leaver itself, which finishVoiceLeave always adds to the audience — so
// every client, this one's own session included, has already torn the old
// membership down. Restoring the client's local voice state on abort
// resurrects a session nobody else believes exists anymore. The fix is to
// leave the client's local state cleared so it agrees with the voice_leave it
// already received; the stale DB row then disagrees with every connected
// client's voiceChID and the periodic sweep reaps it.
func TestVoice_Join_AbortedSwitch_DoesNotResurrectPhantomSession(t *testing.T) {
hub, database := newVoiceHub(t)
user := seedVoiceOwner(t, database, "abort-switch-user")
chanA := seedVoiceChan(t, database, "vc-abort-a")
chanB := seedVoiceChan(t, database, "vc-abort-b")
send := make(chan []byte, 32)
c := ws.NewTestClientWithUser(hub, user, chanA, send)
hub.Register(c)
waitRegistered(t, hub, c)
// Join channel A normally.
hub.HandleMessageForTest(c, voiceJoinMsg(chanA))
drainChanTimeout(send, 30*time.Millisecond)
stateA, _ := database.GetVoiceState(context.Background(), user.ID)
if stateA == nil || stateA.ChannelID != chanA {
t.Fatalf("user should be in channel A, got %+v", stateA)
}
// Simulate the repro from the finding: the client's local join token has
// gone missing (e.g. a prior partial failure) while its voice channel ID
// still agrees with the DB row. leaveVoiceChannelWithRetry's empty-token
// guard then skips the DELETE entirely, so the pre-switch leave silently
// no-ops and the old row survives.
ws.SetClientVoiceStateForTest(c, chanA, "")
// Attempt to switch to channel B. handleVoiceLeave runs first (broadcasts
// voice_leave for chanA to the leaver, per finishVoiceLeave), the DB
// delete is skipped, and handleVoiceJoin's stale-state check aborts the
// switch.
hub.HandleMessageForTest(c, voiceJoinMsg(chanB))
// Confirm the abort branch actually triggered: the old row must still be
// present in the DB.
stillA, _ := database.GetVoiceState(context.Background(), user.ID)
if stillA == nil || stillA.ChannelID != chanA {
t.Fatalf("test setup broken: expected stale row in chanA, got %+v", stillA)
}
if got := ws.GetClientVoiceChIDForTest(c); got != 0 {
t.Errorf("aborted switch resurrected a phantom session: client voice channel = %d, want 0 (voice_leave for chanA was already broadcast to this client, including itself)", got)
}
if hub.SubscribedToVoiceTopicForTest(c, chanA) {
t.Error("aborted switch re-subscribed the client to chanA's voice topic after voice_leave was already broadcast for it")
}
}
// ─── voice leave on disconnect ────────────────────────────────────────────────
// TestVoice_Leave_OnDisconnect verifies that handleVoiceLeave cleans up
+14 -9
View File
@@ -181,15 +181,20 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe
if vs != nil {
slog.Warn("handleVoiceJoin: stale voice state persists after leave, aborting switch",
"user_id", c.userID, "stale_channel", vs.ChannelID, "target_channel", channelID)
// Restore client voice state so the user knows they're still in the
// old channel. The failed leave already dropped the voice-topic
// subscription and key-holder entry, and voice state and topic
// subscription must move as a pair (see clearVoiceAndUnsubscribe)
// — without them the restored session silently misses every
// voice_e2ee relay for its channel.
c.setVoiceState(vs.ChannelID, vs.JoinedAt)
h.pubsub.Subscribe(c, VoiceTopic(vs.ChannelID))
h.updateKeyHolder(vs.ChannelID)
// OC-0034: do NOT restore the client's local voice state here.
// handleVoiceLeave above already broadcast voice_leave for the old
// channel to every client that can see it — including this one,
// since finishVoiceLeave always adds the leaver to the audience —
// so every client, this user's own session included, has already
// torn the old membership down (dispatcher.ts runs leaveVoice on a
// self voice_leave). Restoring c.voiceChID/the topic subscription
// would resurrect a session nobody else believes exists anymore,
// while the stale DB row (this branch's trigger) stays orphaned.
// Leaving the client cleared keeps it consistent with the
// voice_leave it just received: the row now disagrees with every
// connected client's voiceChID, so sweepStaleVoiceStates reaps it
// (re-broadcasting voice_leave, harmlessly) within one tick, and
// the user_id-PK upsert lets the user rejoin immediately.
c.sendMsg(buildErrorMsg(ErrCodeInternal, "voice channel switch failed — please try again"))
return
}
+3 -25
View File
@@ -70,33 +70,11 @@ func (h *Hub) finishVoiceLeave(ctx context.Context, c *Client, oldChID int64, ol
}
// Audience = broadcastVoiceEvent's (READ still-in-the-room) plus the
// leaver themselves. The union of the room's remaining participants is
// what broadcastVoiceEvent provides and must be kept: voice membership is
// gated on CONNECT_VOICE alone, so a participant without READ would
// otherwise miss the departure and keep a stale E2EE key holder. The extra
// term is the leaver: the caller has already cleared their client voice
// leaver themselves: the caller has already cleared their client voice
// state, so that union can no longer see them, yet for a server-initiated
// eviction (revocation sweep, moderator kick/move, token-refresh refusal)
// this voice_leave IS their only teardown signal. Mirrors
// CleanupVoiceForChannel, which appends the evicted participants for
// exactly the same reason.
audience := h.channelReadAudience(ctx, oldChID)
seen := make(map[int64]struct{}, len(audience)+1)
for _, uid := range audience {
seen[uid] = struct{}{}
}
h.mu.RLock()
for uid, other := range h.clients {
if _, ok := seen[uid]; !ok && other.getVoiceChID() == oldChID {
seen[uid] = struct{}{}
audience = append(audience, uid)
}
}
h.mu.RUnlock()
if _, ok := seen[c.userID]; !ok {
audience = append(audience, c.userID)
}
h.broadcastChannelScopedTo(oldChID, buildVoiceLeave(oldChID, c.userID), audience, "voice event")
// this voice_leave IS their only teardown signal.
h.broadcastVoiceEventWithLeaver(ctx, oldChID, buildVoiceLeave(oldChID, c.userID), c.userID)
// Re-elect key holder now that this user has left the channel.
h.updateKeyHolder(oldChID)