From 9e21ea17bbbbcb3e383d09fd3d9106645e13570d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 21 Aug 2026 17:08:50 -0700 Subject: [PATCH] =?UTF-8?q?Fix=20log=20retention=20Settings=20section=20ge?= =?UTF-8?q?tting=20stuck=20on=20"Loading=20hosts=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: handleHosts and partitionTargets both declared their result slices with `var`, so an empty result (no logs old enough yet, or every requested target blocked by a floor) marshaled to JSON `null` instead of `[]` on fields without `omitempty`. The frontend's `.length` access on that `null` threw mid-render, which is why this shipped with the spinner stuck forever instead of the empty state ever painting -- production is freshly deployed with nothing yet older than the default 30-day cutoff, so every user hit this on first load. Also replaces the static "Loading hosts…" text with the existing shimmer Skeleton component for real visual feedback, and adds `?? []` fallbacks in api.ts as a second line of defense. --- api/logretention/handler.go | 27 +++++++++++--- api/logretention/handler_test.go | 56 ++++++++++++++++++++++++++++ web/src/lib/api.ts | 18 ++++++--- web/src/routes/settings/+page.svelte | 22 ++++++++++- 4 files changed, 111 insertions(+), 12 deletions(-) diff --git a/api/logretention/handler.go b/api/logretention/handler.go index e5cae14..9148c03 100644 --- a/api/logretention/handler.go +++ b/api/logretention/handler.go @@ -149,16 +149,26 @@ type blockedTarget struct { // acting on other targets requested in the same call. An owner always // gets everything back as allowed, no query needed. func (h *Handler) partitionTargets(ctx context.Context, role authz.Role, targets []HostService, cutoff time.Time) ([]HostService, []blockedTarget, error) { + // previewResponse.Targets and deleteResponse.DeletedTargets marshal + // this return value without omitempty, so it must never be a nil + // slice: a nil []HostService marshals to JSON `null`, not `[]`, which + // crashes the frontend's `.length` access the same way handleHosts' + // nil hosts slice did. Every return path below -- the owner + // fast-path (targets itself can be nil, e.g. an omitted "targets" + // field), the error path, and the loop -- must produce `[]`, not nil. if role == authz.RoleOwner { - return targets, nil, nil + if targets == nil { + targets = []HostService{} + } + return targets, []blockedTarget{}, nil } floors, err := h.floor.FloorsByHost(ctx) if err != nil { return nil, nil, err } now := time.Now().UTC() - var allowed []HostService - var blocked []blockedTarget + allowed := []HostService{} + blocked := []blockedTarget{} for _, t := range targets { days, hasFloor := floors[t.Host].Effective(t.Service) if !hasFloor { @@ -234,8 +244,15 @@ func (h *Handler) handleHosts(w http.ResponseWriter, r *http.Request) { } // counts is ordered by host (Store.TargetsOlderThan), so contiguous - // rows for the same host can be grouped in one pass. - var hosts []hostEntry + // rows for the same host can be grouped in one pass. Starts as an + // empty (non-nil) slice, not `var hosts []hostEntry` -- a nil slice + // marshals to JSON `null`, not `[]`, and a genuinely empty result + // (no logs at all older than the requested age -- the normal state + // for a freshly-deployed instance) hit exactly that: the frontend's + // `hosts.length` on a `null` response threw mid-render, which is why + // this shipped with "Loading hosts…" stuck forever instead of the + // empty state ever painting. + hosts := []hostEntry{} for _, c := range counts { hf := floors[c.Host] if len(hosts) == 0 || hosts[len(hosts)-1].Host != c.Host { diff --git a/api/logretention/handler_test.go b/api/logretention/handler_test.go index 51235ce..d6e6e5f 100644 --- a/api/logretention/handler_test.go +++ b/api/logretention/handler_test.go @@ -479,6 +479,62 @@ func TestAllTargetsBlockedReturnsZeroCountNotError(t *testing.T) { if len(s.deletedWith) != 0 || len(s.countedWith) != 0 { t.Error("the store must never be called when every requested target is blocked") } + // Regression check for the production "stuck loading" bug: decoding + // through json.Unmarshal above can't tell a JSON `null` apart from + // `[]` (both land as a nil/zero-length Go slice), which is exactly + // how this shipped broken the first time -- deleted_targets has no + // omitempty tag, so it must be a literal `[]` on the wire, not + // `null`, or the frontend's `.length` access on it throws. + if bytes.Contains(rec.Body.Bytes(), []byte(`"deleted_targets":null`)) { + t.Errorf("deleted_targets marshaled as null, not []: %s", rec.Body.String()) + } +} + +// TestHostsWithNoResultsReturnsEmptyArrayNotNull is a regression test +// for the production bug where a freshly-deployed instance (nothing yet +// old enough to be listed) got back `"hosts":null` instead of +// `"hosts":[]` -- Hosts has no omitempty tag, so the frontend's +// `hosts.length` threw mid-render on the null instead of the empty +// state ever painting. See handleHosts' hosts := []hostEntry{} comment. +func TestHostsWithNoResultsReturnsEmptyArrayNotNull(t *testing.T) { + s := &fakeStore{targetList: nil} + h := newTestHandler(s, authz.RoleAdmin) + + rec := doRequest(t, h, "GET", "/logs/retention/hosts?older_than_hours=720") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if bytes.Contains(rec.Body.Bytes(), []byte(`"hosts":null`)) { + t.Errorf("hosts marshaled as null, not []: %s", rec.Body.String()) + } + var resp hostsResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding response: %v", err) + } + if len(resp.Hosts) != 0 { + t.Errorf("hosts = %+v, want empty", resp.Hosts) + } +} + +// TestPreviewAllTargetsBlockedReturnsEmptyArrayNotNull mirrors +// TestAllTargetsBlockedReturnsZeroCountNotError but for the preview +// endpoint, whose Targets field carries the identical no-omitempty risk. +func TestPreviewAllTargetsBlockedReturnsEmptyArrayNotNull(t *testing.T) { + s := &fakeStore{count: 100} + h := NewHandler(discardLogger(), s, fakeFloor{byHost: map[string]HostFloor{ + "web-01": {ServiceDays: map[string]int{"smtp": 90}}, + }}, fakeAuthorizer{role: authz.RoleAdmin}) + + rec := doJSONRequest(t, h, "POST", "/logs/retention/preview", deletionRequest{ + OlderThanHours: hoursForDays(30), + Targets: []HostService{{Host: "web-01", Service: "smtp"}}, + }) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if bytes.Contains(rec.Body.Bytes(), []byte(`"targets":null`)) { + t.Errorf("targets marshaled as null, not []: %s", rec.Body.String()) + } } // TestOwnerBypassesRetentionFloor confirms the whole point of the diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index cb103a9..96e530e 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -521,24 +521,32 @@ export type LogRetentionDeleteResult = { blocked_targets?: BlockedTarget[]; }; +// The `?? []` fallbacks below are a second line of defense, matching +// listUsers' pattern above -- api/logretention/handler.go now always +// sends `[]` rather than `null` for these fields (see its partitionTargets +// and handleHosts comments), but a null response should degrade to an +// empty list here rather than crash the page's `.length` accesses if +// that guarantee ever regresses. export function listRetentionHosts(olderThanHours: number): Promise { - return request(`/logs/retention/hosts?older_than_hours=${olderThanHours}`, { credentials: 'include' }); + return request(`/logs/retention/hosts?older_than_hours=${olderThanHours}`, { + credentials: 'include' + }).then((r) => ({ ...r, hosts: r.hosts ?? [] })); } export function previewLogDeletion(olderThanHours: number, targets: HostService[]): Promise { - return request('/logs/retention/preview', { + return request('/logs/retention/preview', { method: 'POST', credentials: 'include', body: JSON.stringify({ older_than_hours: olderThanHours, targets }) - }); + }).then((r) => ({ ...r, targets: r.targets ?? [] })); } export function deleteLogsOlderThan(olderThanHours: number, targets: HostService[]): Promise { - return request('/logs/retention/delete', { + return request('/logs/retention/delete', { method: 'POST', credentials: 'include', body: JSON.stringify({ older_than_hours: olderThanHours, targets }) - }); + }).then((r) => ({ ...r, deleted_targets: r.deleted_targets ?? [] })); } // --- alerting --------------------------------------------------------- diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 10368eb..ef0aeb3 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -18,6 +18,7 @@ } from '$lib/api'; import { getTheme, setTheme, type Theme } from '$lib/theme.svelte'; import { getDensity, setDensity, type Density } from '$lib/density.svelte'; + import Skeleton from '$lib/components/ui/Skeleton.svelte'; let loading = $state(true); let features = $state({ sso_configured: false, oidc_enabled: false, saml_enabled: false }); @@ -122,7 +123,7 @@ selectedTargets = new Set(); try { const result = await listRetentionHosts(hours); - hosts = result.hosts; + hosts = result.hosts ?? []; } catch (e) { hostsError = e instanceof Error ? e.message : String(e); hosts = []; @@ -304,7 +305,12 @@ {#if hostsError}

{hostsError}

{/if} {#if hostsLoading} -

Loading hosts…

+
+ Loading hosts… + + + +
{:else if hosts.length === 0}

No hosts have logs older than this.

{:else} @@ -459,6 +465,18 @@ .muted { color: var(--color-text-muted); } + .host-picker-loading { + display: flex; + flex-direction: column; + gap: var(--space-2); + } + .sr-only { + position: absolute; + width: 1px; + height: 1px; + overflow: hidden; + clip: rect(0 0 0 0); + } .note { color: var(--color-text-muted); font-size: var(--text-sm);