mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix: correctness fixes across LiveKit voice, client session and transport paths (#1374)
* fix: enhance bugfix workflow documentation with detailed clustering and staging instructions * fix(voice): 6 defect(s) (OC-0001, OC-0006, OC-0009, OC-0010, OC-0015, OC-0029) * fix(voice): 1 defect(s) (OC-0005) * fix(client): 1 defect(s) (OC-0007) * fix(client): 1 defect(s) (OC-0011) * fix(client): 1 defect(s) (OC-0012) * fix(admin): 1 defect(s) (OC-0013) * fix(client): 3 defect(s) (OC-0014, OC-0024, OC-0031) * fix(voice): 1 defect(s) (OC-0018) * fix(voice): 1 defect(s) (OC-0019) * fix(client): 1 defect(s) (OC-0021) * fix(client): 1 defect(s) (OC-0025) * fix(ws): 1 defect(s) (OC-0026) * fix(client): 1 defect(s) (OC-0027) * fix(client): 1 defect(s) (OC-0028) * fix(identity): 1 defect(s) (OC-0030) * fix(voice): 1 defect(s) (OC-0016) * fix(client): 2 defect(s) (OC-0002, OC-0020) OC-0002: chain offer handling behind the announce chain so an offer that arrives immediately behind its sender's announce is not dropped as an unknown peer. OC-0020: retire a departing peer's ECDH key on participant-left so a replayed pre-leave announce cannot overwrite the fresh key they rejoined with. * fix(voice): 1 defect(s) (OC-0008) handleVoiceJoin handed the client its LiveKit token before checking whether the join had been superseded by a concurrent eviction (moderator kick/move, the CONNECT_VOICE revocation sweep, CleanupVoiceForChannel). Those evictors delete the voice_states row, clear the client's in-memory state, and call RemoveParticipant — which no-ops because the join has not reached the SFU yet. The client was left holding a live 5-minute RoomJoin credential for a membership the server had just torn down. Re-check the client's voice state immediately after GenerateToken and withhold the credential if the join was superseded, with a best-effort RemoveParticipant to match every other eviction path. * fix(ws): 2 defect(s) (OC-0017, OC-0022) OC-0017: sweepStaleVoiceStates re-checks the live client immediately before deleting a snapshotted-stale voice_states row. voice_join commits the row before calling c.setVoiceState, so a join that lands inside that window was snapshotted as a ghost and had its just-committed row deleted, leaving the client in voice in memory with no DB row. OC-0022: CleanupVoiceForChannel resolves its voice_leave audience with a variant of channelReadAudience that skips the archived short-circuit. Both production callers archive the channel before evicting, so the plain resolver always returned an empty audience and only the evicted participants learned the call ended. * fix(voice): 1 defect(s) (OC-0023) Camera and screenshare now draw from the same per-channel voice_max_video budget. handleVoiceScreenshareV2 performed no cap check at all, and the camera gate's slot-count subquery counted only `camera = 1` rows, so a screensharing occupant was invisible to it. Both gates now count `camera = 1 OR screenshare = 1` via a shared enableVideoSlot helper. * fix(client): 2 defect(s) (OC-0032, OC-0033) OC-0033: voice_disconnected staleness guard swallowed the kick toast when the sibling voice_leave had already cleared currentChannelId. Treat a cleared store as not-stale. OC-0032: VIDEO_LIMIT rollback assumed the camera, tearing down a working camera and leaving refused screen tracks published. Correlate by envelope id and roll back the kind that was actually refused. * fix(voice): 1 defect(s) (OC-0034) * fix(client): 1 defect(s) (OC-0035) A superseded video-enable id makes rollbackPendingVideo return undefined. The dispatcher's ternary treated undefined as "not screen" and called disableCamera(), tearing down a working camera the user never touched. Return early instead: undefined means there is nothing to roll back. * fix(voice): 1 defect(s) (OC-0036) --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
||||
"crypto/rand"
|
||||
"encoding/hex"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
@@ -474,6 +475,24 @@ func handleLogStream(database *db.DB, ringBuf *RingBuffer) http.HandlerFunc {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
flusher.Flush()
|
||||
|
||||
// This handler streams through the ordinary ResponseWriter (no
|
||||
// Hijack), so it is otherwise subject to http.Server.WriteTimeout:
|
||||
// net/http sets the connection's write deadline exactly once, when
|
||||
// request headers are read, and nothing about writing more data
|
||||
// later extends it. Without clearing it here, every write past that
|
||||
// deadline (including the keepalive ticks below) silently times out
|
||||
// — the caller discards write errors, per SSE convention, since a
|
||||
// client that vanishes is detected via ctx.Done() instead — so the
|
||||
// stream goes silently dead and the client eventually sees the
|
||||
// connection close, then reconnects and replays the full backfill.
|
||||
// SetWriteDeadline(zero) clears the deadline on HTTP/1 and cancels
|
||||
// the per-stream deadline timer on HTTP/2; ErrNotSupported means the
|
||||
// ResponseWriter doesn't sit over a real connection (e.g. in tests),
|
||||
// which is fine to ignore.
|
||||
if err := http.NewResponseController(w).SetWriteDeadline(time.Time{}); err != nil && !errors.Is(err, http.ErrNotSupported) {
|
||||
slog.Warn("log stream: failed to clear write deadline; stream may be cut by WriteTimeout", "err", err)
|
||||
}
|
||||
|
||||
// Snapshot the backfill and subscribe to new entries atomically: the
|
||||
// per-entry principalStillAuthorized() check below is a DB round-trip,
|
||||
// so the backfill loop is slow enough that a Snapshot()-then-Subscribe()
|
||||
@@ -492,7 +511,8 @@ func handleLogStream(database *db.DB, ringBuf *RingBuffer) http.HandlerFunc {
|
||||
}
|
||||
flusher.Flush()
|
||||
|
||||
// Keepalive ticker to avoid WriteTimeout (30s).
|
||||
// Keepalive ticker against intermediary/proxy idle timeouts (the
|
||||
// connection's own WriteTimeout was already neutralized above).
|
||||
keepalive := time.NewTicker(15 * time.Second)
|
||||
defer keepalive.Stop()
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
@@ -161,3 +162,87 @@ func TestHandleLogStream_BackfillStopsAfterAPITokenRevocation(t *testing.T) {
|
||||
t.Fatalf("expected backfill to stop after first entry once the API token was revoked, wrote %d entries; body = %s", writer.writeCount, writer.buffer.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleLogStream_SurvivesServerWriteTimeout pins the bug described in
|
||||
// OC-0013: the handler writes SSE through the ordinary ResponseWriter (no
|
||||
// Hijack), so on a real http.Server the connection's write deadline is set
|
||||
// exactly once, when headers are read (net/http's conn.readRequest), from
|
||||
// srv.WriteTimeout. Nothing in the handler extends that deadline, so once it
|
||||
// elapses every further write on the connection silently times out (the
|
||||
// handler discards write errors) and the client stops receiving anything.
|
||||
//
|
||||
// This must run against a real http.Server (httptest.NewUnstartedServer),
|
||||
// not httptest.NewRequest/httptest.ResponseRecorder, because a
|
||||
// ResponseRecorder has no underlying connection to enforce a write deadline
|
||||
// on and so cannot reproduce the failure.
|
||||
func TestHandleLogStream_SurvivesServerWriteTimeout(t *testing.T) {
|
||||
database := newLogStreamTestDB(t)
|
||||
logBuf := NewRingBuffer(64)
|
||||
|
||||
userID, err := database.CreateUser(context.Background(), "owner", "hash", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
token, err := auth.GenerateToken()
|
||||
if err != nil {
|
||||
t.Fatalf("GenerateToken: %v", err)
|
||||
}
|
||||
tokenHash := auth.HashToken(token)
|
||||
if _, err := database.CreateSession(context.Background(), userID, tokenHash, "test", "127.0.0.1"); err != nil {
|
||||
t.Fatalf("CreateSession: %v", err)
|
||||
}
|
||||
|
||||
ticket, err := logTickets.issue(tokenHash)
|
||||
if err != nil {
|
||||
t.Fatalf("issue ticket: %v", err)
|
||||
}
|
||||
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/logs/stream", handleLogStream(database, logBuf))
|
||||
|
||||
srv := httptest.NewUnstartedServer(mux)
|
||||
// A short stand-in for main.go's srv.WriteTimeout: 30 * time.Second, so
|
||||
// the test doesn't have to wait 30s for the deadline to elapse.
|
||||
srv.Config.WriteTimeout = 200 * time.Millisecond
|
||||
srv.Start()
|
||||
defer srv.Close()
|
||||
|
||||
resp, err := http.Get(srv.URL + "/logs/stream?ticket=" + ticket)
|
||||
if err != nil {
|
||||
t.Fatalf("GET: %v", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200", resp.StatusCode)
|
||||
}
|
||||
|
||||
// Let the connection's write deadline (set once, at request-header time)
|
||||
// elapse before asking the handler to write anything else.
|
||||
time.Sleep(400 * time.Millisecond)
|
||||
|
||||
logBuf.Write(LogEntry{Timestamp: "2026-08-15T00:00:00Z", Level: "info", Message: "after-timeout", Source: "test"})
|
||||
|
||||
type readResult struct {
|
||||
data []byte
|
||||
err error
|
||||
}
|
||||
resultCh := make(chan readResult, 1)
|
||||
body := resp.Body
|
||||
go func() {
|
||||
buf := make([]byte, 4096)
|
||||
n, rerr := body.Read(buf)
|
||||
resultCh <- readResult{data: buf[:n], err: rerr}
|
||||
}()
|
||||
|
||||
select {
|
||||
case res := <-resultCh:
|
||||
if res.err != nil {
|
||||
t.Fatalf("expected the post-timeout log entry to be delivered, got a read error instead (connection was severed by WriteTimeout): %v", res.err)
|
||||
}
|
||||
if !bytes.Contains(res.data, []byte("after-timeout")) {
|
||||
t.Fatalf("expected the post-timeout entry in the stream, got: %q", res.data)
|
||||
}
|
||||
case <-time.After(3 * time.Second):
|
||||
t.Fatal("timed out waiting for the post-WriteTimeout log entry; stream appears severed by http.Server.WriteTimeout")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user