diff --git a/PROGRESS.md b/PROGRESS.md index 0d948eb..f4df715 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -24,6 +24,28 @@ up instantly. Newest status at the top. channel CRUD with full per-channel Opus audio config, user moderation (kick/ban/move/server mute/server deafen/set permissions), and server account management. `dotnet test` of the Windows solution passes. Still to do: DRED/audio-quality polish. +- **Done:** **Fixed "randomly bumped to Lobby" in the Windows client — actors were excluded + from their own state-change broadcasts** (2026-06-17, reported live: a connected client would + intermittently snap from its joined channel back to Lobby in the UI). Root cause was a design + inconsistency, not a disconnect: the server delivered self-initiated state changes (channel + join/leave, stream announce/stop) only as a private `*Result` to the actor and broadcast the + authoritative `UserEvent::UPDATED` to *everyone else*. The core never applied the join result + to its `SessionModel`, so `vc_list_users()` kept self in the old channel; the Windows + `HandleUserUpdated` rebuilds `_currentChannelId` from `vc_list_users()` on **any** user's + UPDATED event, so the next unrelated event (someone joining, announcing/stopping a stream, + being muted) surfaced the stale self-channel → "bumped to Lobby." Flaky because it depended on + other users' activity. **Fix (broad, per the actor-sees-own-changes principle):** the server + now broadcasts these `UserEvent::UPDATED`s to **all** clients including the actor + (`server/src/conn_session.cpp`, `broadcast(…, /*exclude*/ 0)`), and text fan-out now includes + the sender (`resolve_text_targets`), so every client converges via one authoritative path. + The `*Result` is now purely ack/correlation/actor-private payload; response handlers no longer + mutate the local model. Windows client drops its optimistic text echo (the relay comes back) + and renders the sender's own message via `HandleTextMessage`. Documented the + response-vs-broadcast contract in `docs/protocol.md` §6. Registry-level admin broadcasts + (move/mute/kick/channel CRUD) already used `exclude=0` and were correct. `ctest --test-dir + build/m1-dev` — **18/18 green** (PowerShell); Windows `VoiceCat.App` builds 0 warnings. + **Latent, not fixed:** the client never sends the `Ping` keepalive that `docs/protocol.md` §7 + describes (only the server answers pings) — unrelated to this bug, noted for later. - **Done:** **Fixed a *second* silent-playback bug — the playout clock free-ran and drifted off the stream** (2026-06-17, reported live: both `vccli` and the Windows client showed `talking=1/0` correctly on VAD/PTT, mic + screen-share were recognized by peers, but nothing was audible). diff --git a/clients/windows/VoiceCat.App/Forms/MainForm.cs b/clients/windows/VoiceCat.App/Forms/MainForm.cs index f4431c2..0e6272c 100644 --- a/clients/windows/VoiceCat.App/Forms/MainForm.cs +++ b/clients/windows/VoiceCat.App/Forms/MainForm.cs @@ -320,8 +320,9 @@ public partial class MainForm : Form if (ev.Result == VcResult.Ok) { _currentChannelId = ev.ChannelId; - // Server doesn't echo UserJoined/UserUpdated back to the mover — patch our - // own entry in _users so RefreshUserList shows us in the new channel. + // The authoritative UserEvent::UPDATED broadcast also reflects this move, but it + // may arrive after this result — patch our own entry now for instant, flicker-free + // feedback. The later UPDATED is idempotent (sets the same channel). if (_users.TryGetValue(_selfUserId, out var self)) _users[_selfUserId] = self with { ChannelId = ev.ChannelId }; RefreshChannelTree(); @@ -351,7 +352,12 @@ public partial class MainForm : Form .LocalDateTime.ToString("HH:mm") : DateTime.Now.ToString("HH:mm"); string sender = GetNickname(ev.UserId); - string prefix = ev.TextScope == VcTextScope.Private ? "(private) " : ""; + // For a private message, ev.ChannelId carries target_id (the recipient user_id). + // When the server relays our own private message back to us, label it with the + // recipient; an incoming private message just shows "(private)". + string prefix = ev.TextScope == VcTextScope.Private + ? (ev.UserId == _selfUserId ? $"(private to {GetNickname(ev.ChannelId)}) " : "(private) ") + : ""; rtbChat.AppendText($"[{time}] {prefix}{sender}: {ev.Text ?? ""}\n"); rtbChat.ScrollToCaret(); if (ev.TextScope == VcTextScope.Private && ev.UserId != _selfUserId) @@ -882,14 +888,9 @@ public partial class MainForm : Form _client.SendText(scope, targetId, msg); txtCompose.Clear(); - - // Server excludes sender from channel fan-out — echo our own message locally. - string time = DateTime.Now.ToString("HH:mm"); - string prefix = scope == VcTextScope.Private - ? $"(private to {GetNickname(targetId)}) " - : ""; - rtbChat.AppendText($"[{time}] {prefix}{_nickname}: {msg}\n"); - rtbChat.ScrollToCaret(); + // No optimistic echo: the server relays our own message back to us (it no longer + // excludes the sender), so HandleTextMessage renders it through the same path as + // every other message. Echoing here too would double it. } // ── Utility ─────────────────────────────────────────────────────────────── diff --git a/core/src/core/client.cpp b/core/src/core/client.cpp index 99c15fc..662a329 100644 --- a/core/src/core/client.cpp +++ b/core/src/core/client.cpp @@ -532,6 +532,9 @@ void vc_client::handle_channel_event(const voicecat::v1::ChannelEvent& ce) { } void vc_client::handle_join_channel_result(const voicecat::v1::JoinChannelResult& msg) { + // This response only acks the request + carries our private audio config. The + // authoritative channel change reaches us as a UserEvent::UPDATED broadcast (the server + // no longer excludes the mover), which keeps session_model_ in sync via apply_user_event. vc_event ev{}; ev.type = VC_EVENT_JOIN_RESULT; ev.result = msg.ok() ? VC_OK : VC_ERR_PROTOCOL; diff --git a/docs/protocol.md b/docs/protocol.md index 54ffcaf..bba6f54 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -274,6 +274,14 @@ message TextMessage { user, kick, ban, server-mute, set-permission, create/reset/delete account). Error `code`s are an enumerated, stable list. - Fatal conditions send **`Disconnect { code; reason }`** then close the TLS connection. +- **The response is for the request; the broadcast is for the state.** A `*Result` only + acknowledges the actor's request (correlation via `request_id`, error text, and any + actor-private payload — e.g. the channel `AudioConfig` in `JoinChannelResult`). The + resulting *state change* is delivered to **every** connected client **including the actor** + via the normal `UserEvent` / `ChannelEvent` / relayed `TextMessage` path. Clients apply + those events to their local model and never re-derive their own state from a `*Result` + (doing so drifts: the actor would miss its own change and a later event for another user + would surface the stale value). ## 7. Keepalive & timeouts diff --git a/server/src/conn_session.cpp b/server/src/conn_session.cpp index 5a433d5..9fc8ba6 100644 --- a/server/src/conn_session.cpp +++ b/server/src/conn_session.cpp @@ -418,13 +418,16 @@ void ConnSession::handle_join_channel(uint64_t req_id, res->set_channel_id(msg.channel_id()); *res->mutable_audio() = ch->audio(); - // Broadcast that this user changed channel. + // Broadcast that this user changed channel — to everyone INCLUDING the mover. + // The JoinChannelResult only acks the request; this UserEvent is the authoritative + // state change every client (mover included) applies to its local model. Excluding + // the mover here is what made it read a stale self-channel. See docs/protocol.md §6. if (auto updated_user = registry_->user_snapshot_user(uid)) { auto bcast = make_env(); auto* ue = bcast.mutable_user_event(); ue->set_kind(voicecat::v1::UserEvent::UPDATED); *ue->mutable_user() = *updated_user; - registry_->broadcast(bcast, session_id_); + registry_->broadcast(bcast, /*exclude*/ 0); } } send_envelope(env); @@ -439,7 +442,7 @@ void ConnSession::handle_leave_channel() { auto* ue = bcast.mutable_user_event(); ue->set_kind(voicecat::v1::UserEvent::UPDATED); *ue->mutable_user() = *updated_user; - registry_->broadcast(bcast, session_id_); + registry_->broadcast(bcast, /*exclude*/ 0); // include the leaver — same as join } } @@ -544,7 +547,7 @@ void ConnSession::handle_stream_announce(uint64_t req_id, auto* ue = bcast.mutable_user_event(); ue->set_kind(voicecat::v1::UserEvent::UPDATED); *ue->mutable_user() = *updated; - registry_->broadcast(bcast, session_id_); + registry_->broadcast(bcast, /*exclude*/ 0); // include announcer; result only acks } } @@ -560,7 +563,7 @@ void ConnSession::handle_stream_stop(const voicecat::v1::StreamStop& msg) { auto* ue = bcast.mutable_user_event(); ue->set_kind(voicecat::v1::UserEvent::UPDATED); *ue->mutable_user() = *updated; - registry_->broadcast(bcast, session_id_); + registry_->broadcast(bcast, /*exclude*/ 0); // include the stopper } } diff --git a/server/src/session_registry.cpp b/server/src/session_registry.cpp index 6868d92..df28bfc 100644 --- a/server/src/session_registry.cpp +++ b/server/src/session_registry.cpp @@ -163,24 +163,27 @@ std::vector> SessionRegistry::resolve_text_targets( std::shared_lock lk(mu_); std::vector> targets; + auto add_session = [&](uint64_t sid) { + auto sit = sessions_.find(sid); + if (sit == sessions_.end()) return; + if (auto sess = sit->second.lock()) targets.push_back(sess); + }; + if (scope == voicecat::v1::TEXT_CHANNEL) { - // Find channel_id of the target, then all users in that channel + // Fan out to everyone in the target channel, INCLUDING the sender, so the sender's + // own client renders the message through the same authoritative relay everyone else + // gets (no optimistic local echo). See docs/protocol.md §6. for (auto& [uid, entry] : users_) { if (entry.proto.channel_id() != target_id) continue; - if (entry.session_id == sender_session_id) continue; - auto sit = sessions_.find(entry.session_id); - if (sit == sessions_.end()) continue; - if (auto sess = sit->second.lock()) targets.push_back(sess); + add_session(entry.session_id); } } else if (scope == voicecat::v1::TEXT_PRIVATE) { - // target_id is user_id + // target_id is the recipient user_id: deliver to the recipient and echo to the + // sender (skip the echo if they messaged themselves, to avoid a duplicate). auto user_it = users_.find(target_id); - if (user_it != users_.end()) { - auto sit = sessions_.find(user_it->second.session_id); - if (sit != sessions_.end()) { - if (auto sess = sit->second.lock()) targets.push_back(sess); - } - } + if (user_it != users_.end()) add_session(user_it->second.session_id); + if (user_it == users_.end() || user_it->second.session_id != sender_session_id) + add_session(sender_session_id); } return targets; }