From b43b7ef06b939b28d86a3af1f1830cb3398f2f01 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:23:03 -0400 Subject: [PATCH] fix(activity): resolve client identity without polluting client_version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the client build/channel work. Fourteen findings; the substantive ones: The v3 body fallback took client_playback_context.app_version whenever the header was absent. The web player sends the literal "web" there and sends no X-Silo-Client, so every browser session would have stamped client_version="web" — the one field the contract promises is semver and the field a future minimum-version gate has to key on. client_playback_context carries no app name, so the body can never identify a nameless client anyway; the fallback now applies only to a client that sent X-Silo-Client, and a test pins the "web" case. An over-long app_build or app_channel in the start body failed the whole request with 400 while the same value in a header was silently clamped — an opaque diagnostic label could refuse playback. validateCapabilitiesV3 now clamps both with the same helper the header path uses, which is what the docs already claimed. Route events posted out of band resolved identity from headers only, so a client reporting its build in the start body attributed plan_selected to a build and every later event of the same attempt to none. They now fill empty fields from the session, as the replan path already did. playbackClientFullDisplayName discarded build and channel whenever the client reported no name, so the new Client card could never show a build for a user-agent-labelled session. It now qualifies whatever label the compact formatter resolved, which also drops its duplicated name+version assembly. normalizeClientMetadataValue truncated by bytes; a multi-byte header value cut mid-rune yields invalid UTF-8, which Postgres rejects — and the per-node session upserts share one transaction, so one malformed client string would fail that whole node's sync. It now clamps on a rune boundary. replan-request.schema.json never got app_build/app_channel even though ReplanRequestV3 reuses ClientPlaybackContextV3 and validates the same bounds. A new contract test asserts every $def the two request schemas share is identical, so the copies cannot drift again. Also: the four client log attrs move to ClientInfo.LogAttrs(), which is now their single definition and omits fields the client did not report rather than persisting empty keys into opslog; startPlannedPlaybackV3 takes the resolved identity instead of re-parsing the headers; client_label_full is omitted when it would repeat client_label; getSessionClientLabelFull delegates to getSessionClientLabel instead of re-implementing it; the Activity search matches the exact label so a build number is findable; and the web ClientPlaybackContextV3 type mirrors the two new optional fields. Co-Authored-By: Claude Opus 5 --- .../playback-v3/v3/replan-request.schema.json | 8 ++ docs/settings-api.md | 19 +++-- internal/api/handlers/playback.go | 6 +- internal/api/handlers/playback_sessions.go | 25 +++--- .../api/handlers/playback_sessions_test.go | 77 ++++++++++++++++++- internal/api/handlers/playback_v3.go | 68 ++++++++++++---- internal/playback/contract/contract_test.go | 28 +++++++ internal/playback/protocol_v3.go | 9 ++- internal/playback/session.go | 46 ++++++++++- web/src/pages/AdminActivity.tsx | 5 +- web/src/pages/adminActivityPresentation.ts | 19 +---- web/src/player/protocol-v3.ts | 8 ++ 12 files changed, 262 insertions(+), 56 deletions(-) diff --git a/docs/design/schemas/playback-v3/v3/replan-request.schema.json b/docs/design/schemas/playback-v3/v3/replan-request.schema.json index bb4c6d55..3b22567e 100644 --- a/docs/design/schemas/playback-v3/v3/replan-request.schema.json +++ b/docs/design/schemas/playback-v3/v3/replan-request.schema.json @@ -580,6 +580,14 @@ "type": "string", "maxLength": 64 }, + "app_build": { + "type": "string", + "maxLength": 64 + }, + "app_channel": { + "type": "string", + "maxLength": 32 + }, "device": { "type": "object", "description": "Manufacturer and model stay first-class because the device-quirk registry matches on them. Everything platform-specific travels in platform_details as opaque bounded strings.", diff --git a/docs/settings-api.md b/docs/settings-api.md index 793d7596..595d2c1b 100644 --- a/docs/settings-api.md +++ b/docs/settings-api.md @@ -63,12 +63,19 @@ carry them so a report can be tied to an exact build. | `X-Silo-Client-Build` | 64 | Opaque per-platform build identifier (Android `versionCode`, Apple `CFBundleVersion`). Never parsed or compared. | | `X-Silo-Client-Channel` | 32 | Opaque distribution channel: `release`, `beta`, `sideload`, `dev`. Stored verbatim; `release` is not displayed. | -Values are trimmed and truncated to the clamp above; nothing is validated -against an enum, so a client may introduce a new channel without a server -change. Protocol-v3 `POST /playback/start` accepts -`client_playback_context.app_version`, `.app_build`, and `.app_channel` as a -body-level fallback for clients that cannot set the headers on every request; -the headers win when both are present. +Values are trimmed and truncated to the clamp above — never rejected, on either +route, because an identity label must not be able to fail a playback start. +Nothing is validated against an enum either, so a client may introduce a new +channel without a server change. + +Protocol-v3 `POST /playback/start` accepts `client_playback_context.app_version`, +`.app_build`, and `.app_channel` as a body-level fallback for clients that cannot +set the headers on every request. The headers win field by field when both are +present, and the fallback applies **only to a client that sent `X-Silo-Client`**: +`client_playback_context` carries no app name, so nothing in the body can +identify a client that did not name itself — such a session is labelled from its +user agent, and its `app_version` is a free-form platform string rather than the +marketing version `client_version` promises. ## Remote scopes diff --git a/internal/api/handlers/playback.go b/internal/api/handlers/playback.go index fd7ea4ff..e895374e 100644 --- a/internal/api/handlers/playback.go +++ b/internal/api/handlers/playback.go @@ -952,9 +952,9 @@ func (h *PlaybackHandler) handleExpiredSession(session *playback.Session) { } sessionCopy := *session go func() { - slog.Info("expired inactive playback session", "session", sessionCopy.ID, "playback_session_id", sessionCopy.ID, - "client_name", sessionCopy.ClientName, "client_version", sessionCopy.ClientVersion, - "client_build", sessionCopy.ClientBuild, "client_channel", sessionCopy.ClientChannel) + slog.Info("expired inactive playback session", append([]any{ + "session", sessionCopy.ID, "playback_session_id", sessionCopy.ID, + }, sessionCopy.ClientInfo().LogAttrs()...)...) // Expiry is a liveness reap, not a user stop — keep the recipe card so a // resume reconstructs under the same id (the card's own TTL reaps it if // the session is truly abandoned). diff --git a/internal/api/handlers/playback_sessions.go b/internal/api/handlers/playback_sessions.go index 1cc9d759..97f4cefc 100644 --- a/internal/api/handlers/playback_sessions.go +++ b/internal/api/handlers/playback_sessions.go @@ -266,7 +266,13 @@ func (l *PlaybackSessionsLoader) Load( s.SourceBitrateKbps = sourceBitrateKbps s.SourceAudioChannels = sourceAudioChannels s.ClientLabel = playbackClientDisplayName(s.ClientName, s.ClientVersion, s.ClientUserAgent) - s.ClientLabelFull = playbackClientFullDisplayName(s.ClientName, s.ClientVersion, s.ClientBuild, s.ClientChannel, s.ClientUserAgent) + // Only worth the bytes when it says more than the compact label — which + // it does not for any client without a build or a non-release channel, + // i.e. most of a 200-row page. Clients read client_label when + // client_label_full is absent, so omitting the duplicate costs nothing. + if full := playbackClientFullDisplayName(s.ClientName, s.ClientVersion, s.ClientBuild, s.ClientChannel, s.ClientUserAgent); full != s.ClientLabel { + s.ClientLabelFull = full + } enrichPlaybackSessionRow(&s, audioTracksJSON) sessions = append(sessions, s) } @@ -463,17 +469,14 @@ func firstNonEmptyValue(values ...string) string { // playbackClientFullDisplayName renders the client's exact identity — version, // opaque build, and non-default channel — for surfaces that can afford the -// width (expanded session details, tooltips). Clients that report no name fall -// back to the compact user-agent label, which is all that can be derived there. +// width (expanded session details, tooltips). The name-and-version half is +// whatever the compact label resolved, so a client identified only by its user +// agent still gets its build named: a client that bothered to report a build +// has earned having it displayed, whether or not it also named itself. func playbackClientFullDisplayName(name, version, build, channel, userAgent string) string { - name = strings.TrimSpace(name) - if name == "" { - return playbackClientDisplayName(name, version, userAgent) - } - - label := name - if version = strings.TrimSpace(version); version != "" { - label += " " + version + label := playbackClientDisplayName(name, version, userAgent) + if label == "" { + return "" } build = strings.TrimSpace(build) diff --git a/internal/api/handlers/playback_sessions_test.go b/internal/api/handlers/playback_sessions_test.go index b4fae920..2bcbd0b6 100644 --- a/internal/api/handlers/playback_sessions_test.go +++ b/internal/api/handlers/playback_sessions_test.go @@ -5,6 +5,8 @@ import ( "net/http" "net/http/httptest" "testing" + + "github.com/Silo-Server/silo-server/internal/playback" ) func TestSessionComponentDecisionLabelsCopiedAudioDuringHLSAsRemux(t *testing.T) { @@ -301,10 +303,18 @@ func TestPlaybackClientFullDisplayName(t *testing.T) { want: "Silo tvOS 1.0.0 (build 2026.08.13-abcdef)", }, { - name: "no client name falls back to user agent label", + // A client that reports a build but not a name is still named by its + // build: the user-agent label supplies the product half, and dropping + // the qualifiers would hide the exact field this surface exists for. + name: "no client name keeps build and channel on the user agent label", userAgent: "Mozilla/5.0 (X11; Linux x86_64) Chrome/120.0.0.0 Safari/537.36", build: "5", channel: "dev", + want: "Chrome 120 (build 5, dev)", + }, + { + name: "no client name and no qualifiers keeps the plain user agent label", + userAgent: "Mozilla/5.0 (X11; Linux x86_64) Chrome/120.0.0.0 Safari/537.36", want: "Chrome 120", }, { @@ -322,6 +332,71 @@ func TestPlaybackClientFullDisplayName(t *testing.T) { } } +func TestPlaybackClientInfoForStartV3(t *testing.T) { + cases := []struct { + name string + headers map[string]string + context playback.ClientPlaybackContextV3 + want playback.ClientInfo + }{ + { + name: "headers win over the body", + headers: map[string]string{"X-Silo-Client": "Silo iOS", "X-Silo-Client-Version": "2.1.0", "X-Silo-Client-Build": "9", "X-Silo-Client-Channel": "beta"}, + context: playback.ClientPlaybackContextV3{AppVersion: "1.0.0", AppBuild: "1", AppChannel: "release"}, + want: playback.ClientInfo{Name: "Silo iOS", Version: "2.1.0", Build: "9", Channel: "beta"}, + }, + { + // The fallback is per field, not per struct: a client may set the + // name and version headers on every request and still report the + // build it only knows at start time in the body. + name: "body fills only the fields the headers omit", + headers: map[string]string{"X-Silo-Client": "Silo tvOS", "X-Silo-Client-Version": "2.1.0"}, + context: playback.ClientPlaybackContextV3{AppVersion: "1.0.0", AppBuild: "77", AppChannel: "sideload"}, + want: playback.ClientInfo{Name: "Silo tvOS", Version: "2.1.0", Build: "77", Channel: "sideload"}, + }, + { + name: "body supplies everything but the name", + headers: map[string]string{"X-Silo-Client": "Silo Android TV"}, + context: playback.ClientPlaybackContextV3{AppVersion: "1.0.0", AppBuild: "5", AppChannel: "dev"}, + want: playback.ClientInfo{Name: "Silo Android TV", Version: "1.0.0", Build: "5", Channel: "dev"}, + }, + { + // The regression this guards: the web player reports the literal + // "web" as its app_version and sends no X-Silo-Client. Taking the + // body anyway would stamp "web" onto client_version — the one field + // the contract promises is a marketing version — for every browser + // session, and onto every route event and decision log with it. + name: "nameless client keeps the body out of client_version", + context: playback.ClientPlaybackContextV3{AppVersion: "web"}, + want: playback.ClientInfo{}, + }, + { + name: "nameless client takes no build or channel either", + context: playback.ClientPlaybackContextV3{AppVersion: "web", AppBuild: "5", AppChannel: "dev"}, + want: playback.ClientInfo{}, + }, + { + name: "whitespace-only header falls through to the body", + headers: map[string]string{"X-Silo-Client": "Silo iOS", "X-Silo-Client-Build": " "}, + context: playback.ClientPlaybackContextV3{AppBuild: " 42 "}, + want: playback.ClientInfo{Name: "Silo iOS", Build: "42"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/v1/playback/start", nil) + for name, value := range tc.headers { + req.Header.Set(name, value) + } + got := playbackClientInfoForStartV3(req, tc.context) + got.UserAgent = "" + if got != tc.want { + t.Fatalf("playbackClientInfoForStartV3() = %+v, want %+v", got, tc.want) + } + }) + } +} + func TestEnrichPlaybackSessionRowUsesCompatOrigin(t *testing.T) { row := playbackSessionRow{ ClientName: "Unrecognized Client", diff --git a/internal/api/handlers/playback_v3.go b/internal/api/handlers/playback_v3.go index 67d1e014..767477ee 100644 --- a/internal/api/handlers/playback_v3.go +++ b/internal/api/handlers/playback_v3.go @@ -462,16 +462,13 @@ func (h *PlaybackHandler) handleStartPlaybackV3(w http.ResponseWriter, r *http.R // user which version they are running. clientInfo := playbackClientInfoForStartV3(r, req.ClientPlaybackContext) if result.Terminal != nil { - slog.InfoContext(r.Context(), "playback plan decided", "component", "playback", + slog.InfoContext(r.Context(), "playback plan decided", append([]any{ + "component", "playback", "outcome", "terminal", "reason", result.Terminal.Reason, "file_id", effectiveFile.ID, "quality_preference", req.QualityPreference, - "client_name", clientInfo.Name, - "client_version", clientInfo.Version, - "client_build", clientInfo.Build, - "client_channel", clientInfo.Channel, - ) + }, clientInfo.LogAttrs()...)...) response, persistErr := h.persistTerminalStartDecisionV3(r.Context(), userID, profileID, req, requestDigests, requestedFile.ID, effectiveFile.ID, playback.NewTerminalResponseV3(result.Terminal.Reason, result.Terminal.Message, result.Terminal.Retryable)) if persistErr != nil { writeStartAttemptPersistenceErrorV3(w, persistErr) @@ -486,7 +483,8 @@ func (h *PlaybackHandler) handleStartPlaybackV3(w http.ResponseWriter, r *http.R // One line per plan decision so route selection is reconstructible from // server logs alone (finding a mis-planned route previously required // correlating client logcat, ffmpeg commands, and session rows). - slog.InfoContext(r.Context(), "playback plan decided", "component", "playback", + slog.InfoContext(r.Context(), "playback plan decided", append([]any{ + "component", "playback", "outcome", "plan", "decision_reason", result.Plan.DecisionReason, "delivery", result.Plan.Delivery, @@ -499,13 +497,9 @@ func (h *PlaybackHandler) handleStartPlaybackV3(w http.ResponseWriter, r *http.R "target_bitrate_kbps", result.TargetBitrateKbps, "quality_preference", req.QualityPreference, "bandwidth_estimate_kbps", intOrZeroHandlerV3(req.BandwidthEstimateKbps), - "client_name", clientInfo.Name, - "client_version", clientInfo.Version, - "client_build", clientInfo.Build, - "client_channel", clientInfo.Channel, - ) + }, clientInfo.LogAttrs()...)...) result.Plan.DegradationWarnings = append(result.Plan.DegradationWarnings, warnings...) - response, statusErr := h.startPlannedPlaybackV3(r, userID, profileID, req, requestDigests, requestedFile, effectiveFile, audioIndex, result) + response, statusErr := h.startPlannedPlaybackV3(r, userID, profileID, req, requestDigests, requestedFile, effectiveFile, audioIndex, result, clientInfo) if statusErr != nil { if statusErr.reason == "playback_attempt_reused" { writeError(w, http.StatusConflict, "playback_attempt_reused", statusErr.message) @@ -535,8 +529,19 @@ type playbackStartRequestDigestsV3 struct { // every request; the start body's client_playback_context is the fallback for // clients that report their app identity only there. All values stay opaque — // they are trimmed and length-clamped when the session stamps them. +// +// The fallback applies only to a client that named itself. client_playback_context +// carries no app name, so nothing in the body can identify a nameless client +// anyway — it is labelled from its user agent, and its app_version is a +// free-form platform string rather than the marketing version client_version +// promises. The web player, for one, reports the literal "web" there; taking it +// unconditionally would write "web" into the one field that is contractually +// semver, on every browser session. func playbackClientInfoForStartV3(r *http.Request, clientContext playback.ClientPlaybackContextV3) playback.ClientInfo { info := playbackClientInfoFromRequest(r) + if info.Name == "" { + return info + } if info.Version == "" { info.Version = strings.TrimSpace(clientContext.AppVersion) } @@ -549,6 +554,38 @@ func playbackClientInfoForStartV3(r *http.Request, clientContext playback.Client return info } +// playbackClientInfoWithSessionFallbackV3 completes a header-derived identity +// from the session the event belongs to. Route events posted out of band carry +// no client_playback_context, so without this a client that reports its build +// only in the start body would attribute its plan_selected event to a build and +// every later event of the same attempt to none. +func (h *PlaybackHandler) playbackClientInfoWithSessionFallbackV3(sessionID string, info playback.ClientInfo) playback.ClientInfo { + if sessionID == "" || h.sessionMgr == nil { + return info + } + if info.Name != "" && info.Version != "" && info.Build != "" && info.Channel != "" { + return info + } + session, err := h.sessionMgr.GetSession(sessionID) + if err != nil || session == nil { + return info + } + stamped := session.ClientInfo() + if info.Name == "" { + info.Name = stamped.Name + } + if info.Version == "" { + info.Version = stamped.Version + } + if info.Build == "" { + info.Build = stamped.Build + } + if info.Channel == "" { + info.Channel = stamped.Channel + } + return info +} + // newPlaybackStartRequestDigestsV3 fingerprints both the body and normalized // device identity because either can change the selected playback plan. It // also retains the pre-device digest while attempts written by an older @@ -569,7 +606,7 @@ func (d playbackStartRequestDigestsV3) matches(stored string) bool { return stored == "" || stored == d.current || stored == d.legacy } -func (h *PlaybackHandler) startPlannedPlaybackV3(r *http.Request, userID int, profileID string, req playback.StartRequestV3, requestDigests playbackStartRequestDigestsV3, requestedFile, effectiveFile *models.MediaFile, audioIndex int, result playback.PlannerResultV3) (playback.DecisionResponseV3, *transportErrorV3) { +func (h *PlaybackHandler) startPlannedPlaybackV3(r *http.Request, userID int, profileID string, req playback.StartRequestV3, requestDigests playbackStartRequestDigestsV3, requestedFile, effectiveFile *models.MediaFile, audioIndex int, result playback.PlannerResultV3, clientInfo playback.ClientInfo) (playback.DecisionResponseV3, *transportErrorV3) { if result.Plan == nil { return playback.DecisionResponseV3{}, &transportErrorV3{reason: "internal_error", message: "The server produced no playback plan."} } @@ -582,7 +619,6 @@ func (h *PlaybackHandler) startPlannedPlaybackV3(r *http.Request, userID int, pr return playback.DecisionResponseV3{}, &transportErrorV3{reason: reason, message: "The selected server adaptation is disabled for this user."} } } - clientInfo := playbackClientInfoForStartV3(r, req.ClientPlaybackContext) ctx := playback.WithClientInfo(r.Context(), clientInfo) var session *playback.Session var err error @@ -2738,7 +2774,7 @@ func (h *PlaybackHandler) HandlePlaybackRouteEventV3(w http.ResponseWriter, r *h return } event.Diagnostics = sanitizeDiagnosticsV3(event.Diagnostics) - client := playbackClientInfoFromRequest(r) + client := h.playbackClientInfoWithSessionFallbackV3(firstNonEmptyValue(event.SessionID, identity.SessionID), playbackClientInfoFromRequest(r)) h.enqueueRouteEventV3(playback.RouteEventRecordV3{RouteEventV3: event, UserID: userID, ProfileID: profileID, ClientName: client.Name, ClientVersion: client.Version, ClientBuild: client.Build, ClientChannel: client.Channel, ClientModel: event.Diagnostics["device_model"]}) w.WriteHeader(http.StatusAccepted) } diff --git a/internal/playback/contract/contract_test.go b/internal/playback/contract/contract_test.go index 5b8414aa..87829c93 100644 --- a/internal/playback/contract/contract_test.go +++ b/internal/playback/contract/contract_test.go @@ -512,6 +512,34 @@ func TestRequestSchemaRequiredFieldsMatchValidators(t *testing.T) { assertStringsEqual(t, "route_event.required", schemaStrings(t, routeEvent, "required"), []string{"protocol_version", "playback_attempt_id", "event"}) } +// TestRequestSchemasShareIdenticalDefs pins the one thing two copies of a +// definition can never be trusted to keep on their own. Start and replan +// deserialize the *same* Go types — ClientCodecCapabilitiesV3 and +// ClientPlaybackContextV3 — through the same validator, so any $def they both +// declare must be byte-identical. Adding a field to one schema and not the +// other compiles, validates, and ships a contract that lies about one of the +// two endpoints; this is what catches it. +func TestRequestSchemasShareIdenticalDefs(t *testing.T) { + start := schemaValue(t, mustReadObject(t, filepath.Join(schemaRootV3, "v3", "start-request.schema.json")), "$defs").(map[string]any) + replan := schemaValue(t, mustReadObject(t, filepath.Join(schemaRootV3, "v3", "replan-request.schema.json")), "$defs").(map[string]any) + + shared := make([]string, 0, len(start)) + for name := range start { + if _, ok := replan[name]; ok { + shared = append(shared, name) + } + } + sort.Strings(shared) + if !slices.Contains(shared, "client_playback_context") || !slices.Contains(shared, "client_capabilities") { + t.Fatalf("shared $defs = %v, want the client contract definitions in both request schemas", shared) + } + for _, name := range shared { + if !reflect.DeepEqual(start[name], replan[name]) { + t.Errorf("$defs.%s differs between start-request and replan-request; both deserialize the same Go type", name) + } + } +} + func compileSchemasV3(t *testing.T) map[string]*jsonschema.Schema { t.Helper() diff --git a/internal/playback/protocol_v3.go b/internal/playback/protocol_v3.go index 7c5a0f4f..8263d085 100644 --- a/internal/playback/protocol_v3.go +++ b/internal/playback/protocol_v3.go @@ -892,9 +892,16 @@ func validateCapabilitiesV3(c *ClientCodecCapabilitiesV3, ctx *ClientPlaybackCon if !validCapabilityEvidenceV3(c.AudioEvidence) { return errors.New("audio_evidence is required and must be exact, platform_attested, or declared") } - if len(c.CodecsVideo) > 64 || len(c.CodecsVideoHardware) > 64 || len(c.CodecsAudio) > 64 || len(c.Containers) > 64 || len(c.VideoDecode) > 64 || len(ctx.Deliveries) > 16 || len(ctx.Device.Platform) > 32 || len(ctx.FormFactor) > 32 || len(ctx.AppVersion) > 64 || len(ctx.AppBuild) > 64 || len(ctx.AppChannel) > 32 { + if len(c.CodecsVideo) > 64 || len(c.CodecsVideoHardware) > 64 || len(c.CodecsAudio) > 64 || len(c.Containers) > 64 || len(c.VideoDecode) > 64 || len(ctx.Deliveries) > 16 || len(ctx.Device.Platform) > 32 || len(ctx.FormFactor) > 32 || len(ctx.AppVersion) > 64 { return errors.New("capability list exceeds supported size") } + // Build and channel are opaque diagnostic labels, so an over-long value is + // worth clamping and never worth refusing playback over. The header route + // (X-Silo-Client-Build / -Channel) clamps with the same helper; rejecting + // here would mean the same string plays from a header and 400s from the + // body. + ctx.AppBuild = normalizeClientMetadataValue(ctx.AppBuild, 64) + ctx.AppChannel = normalizeClientMetadataValue(ctx.AppChannel, 32) deviceValues := []string{ ctx.Device.OSVersion, ctx.Device.Manufacturer, ctx.Device.Model, ctx.Output.CurrentSink, ctx.Output.SinkType, ctx.Output.OutputContextID, diff --git a/internal/playback/session.go b/internal/playback/session.go index 49e4b2f4..8d2bd4ba 100644 --- a/internal/playback/session.go +++ b/internal/playback/session.go @@ -163,6 +163,46 @@ type ClientInfo struct { IsCompat bool } +// LogAttrs renders the app identity as slog key/value pairs, skipping the +// fields the client did not report. This is the single definition of those log +// keys — every playback decision and the session-expiry line share it, so a +// rename cannot leave one surface keyed differently from another. Skipping +// empty values matters as much: browsers and Jellyfin-ecosystem clients report +// none of them, and opslog persists the attrs it is handed, so emitting four +// empty keys per decision would grow /admin/logs for no diagnostic value. +func (c ClientInfo) LogAttrs() []any { + attrs := make([]any, 0, 8) + for _, pair := range [...]struct{ key, value string }{ + {"client_name", c.Name}, + {"client_version", c.Version}, + {"client_build", c.Build}, + {"client_channel", c.Channel}, + } { + if pair.value != "" { + attrs = append(attrs, pair.key, pair.value) + } + } + return attrs +} + +// ClientInfo returns the app identity stamped on the session when it was +// created. Surfaces that only hold a session — expiry logging, route events +// posted out of band — recover the reporting client through this instead of +// re-reading request headers that may no longer be present. +func (s *Session) ClientInfo() ClientInfo { + if s == nil { + return ClientInfo{} + } + return ClientInfo{ + Name: s.ClientName, + Version: s.ClientVersion, + Build: s.ClientBuild, + Channel: s.ClientChannel, + UserAgent: s.ClientUserAgent, + IsCompat: s.IsJellyfinCompat, + } +} + // WithClientInfo stores playback client metadata on a context. func WithClientInfo(ctx context.Context, info ClientInfo) context.Context { if ctx == nil { @@ -314,7 +354,11 @@ func (m *SessionManager) SetExpirationHook(fn func(*Session)) { func normalizeClientMetadataValue(value string, maxLen int) string { value = strings.TrimSpace(value) if maxLen > 0 && len(value) > maxLen { - value = value[:maxLen] + // Clamp on a rune boundary. Header values may carry multi-byte UTF-8, and + // a mid-rune byte slice produces a string Postgres refuses outright — the + // per-node session upserts share one transaction, so a single malformed + // client string would fail that whole node's sync rather than one row. + value = strings.ToValidUTF8(value[:maxLen], "") } return value } diff --git a/web/src/pages/AdminActivity.tsx b/web/src/pages/AdminActivity.tsx index c7030a24..15b3a59d 100644 --- a/web/src/pages/AdminActivity.tsx +++ b/web/src/pages/AdminActivity.tsx @@ -150,7 +150,10 @@ export default function AdminActivity() { s.media_title?.toLowerCase().includes(q) || s.series_name?.toLowerCase().includes(q) || s.episode_name?.toLowerCase().includes(q) || - getSessionClientLabel(s).toLowerCase().includes(q) || + // Search the exact label, not the compact one: "which sessions are on + // build 5?" is the question this identity exists to answer, and the + // compact label deliberately omits the build. + getSessionClientLabelFull(s).toLowerCase().includes(q) || s.client_user_agent?.toLowerCase().includes(q) || s.client_ip?.toLowerCase().includes(q), ); diff --git a/web/src/pages/adminActivityPresentation.ts b/web/src/pages/adminActivityPresentation.ts index 0042f579..9492a5a7 100644 --- a/web/src/pages/adminActivityPresentation.ts +++ b/web/src/pages/adminActivityPresentation.ts @@ -252,22 +252,9 @@ export function getSessionClientLabel(session: AdminSession): string { * replace it in a fixed-width row. */ export function getSessionClientLabelFull(session: AdminSession): string { - const fullLabel = session.client_label_full?.trim(); - if (fullLabel) { - return fullLabel; - } - - const label = session.client_label?.trim(); - if (label) { - return label; - } - - const clientName = session.client_name?.trim(); - const clientVersion = session.client_version?.trim(); - if (clientName && clientVersion) { - return `${clientName} ${clientVersion}`; - } - return clientName || ""; + // The server omits client_label_full when it would repeat client_label, and + // older servers never send it at all — both degrade to the compact label. + return session.client_label_full?.trim() || getSessionClientLabel(session); } export function formatSourceContainerSummary(session: AdminSession): string { diff --git a/web/src/player/protocol-v3.ts b/web/src/player/protocol-v3.ts index 0f20f942..84d43969 100644 --- a/web/src/player/protocol-v3.ts +++ b/web/src/player/protocol-v3.ts @@ -252,6 +252,14 @@ export interface ClientPlaybackContextV3 { protocol_version: number; form_factor: string; app_version: string; + /** + * Opaque per-platform build identifier and distribution channel — the + * body-level fallback for the `X-Silo-Client-Build` / `X-Silo-Client-Channel` + * headers. The server stores both verbatim and never parses or compares + * them. The web player has no build concept and omits them. + */ + app_build?: string; + app_channel?: string; device: DeviceContextV3; output: OutputContextV3; /**