Fix log retention Settings section getting stuck on "Loading hosts…"
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.
This commit is contained in:
@@ -149,16 +149,26 @@ type blockedTarget struct {
|
|||||||
// acting on other targets requested in the same call. An owner always
|
// acting on other targets requested in the same call. An owner always
|
||||||
// gets everything back as allowed, no query needed.
|
// 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) {
|
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 {
|
if role == authz.RoleOwner {
|
||||||
return targets, nil, nil
|
if targets == nil {
|
||||||
|
targets = []HostService{}
|
||||||
|
}
|
||||||
|
return targets, []blockedTarget{}, nil
|
||||||
}
|
}
|
||||||
floors, err := h.floor.FloorsByHost(ctx)
|
floors, err := h.floor.FloorsByHost(ctx)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, nil, err
|
return nil, nil, err
|
||||||
}
|
}
|
||||||
now := time.Now().UTC()
|
now := time.Now().UTC()
|
||||||
var allowed []HostService
|
allowed := []HostService{}
|
||||||
var blocked []blockedTarget
|
blocked := []blockedTarget{}
|
||||||
for _, t := range targets {
|
for _, t := range targets {
|
||||||
days, hasFloor := floors[t.Host].Effective(t.Service)
|
days, hasFloor := floors[t.Host].Effective(t.Service)
|
||||||
if !hasFloor {
|
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
|
// counts is ordered by host (Store.TargetsOlderThan), so contiguous
|
||||||
// rows for the same host can be grouped in one pass.
|
// rows for the same host can be grouped in one pass. Starts as an
|
||||||
var hosts []hostEntry
|
// 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 {
|
for _, c := range counts {
|
||||||
hf := floors[c.Host]
|
hf := floors[c.Host]
|
||||||
if len(hosts) == 0 || hosts[len(hosts)-1].Host != c.Host {
|
if len(hosts) == 0 || hosts[len(hosts)-1].Host != c.Host {
|
||||||
|
|||||||
@@ -479,6 +479,62 @@ func TestAllTargetsBlockedReturnsZeroCountNotError(t *testing.T) {
|
|||||||
if len(s.deletedWith) != 0 || len(s.countedWith) != 0 {
|
if len(s.deletedWith) != 0 || len(s.countedWith) != 0 {
|
||||||
t.Error("the store must never be called when every requested target is blocked")
|
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
|
// TestOwnerBypassesRetentionFloor confirms the whole point of the
|
||||||
|
|||||||
+13
-5
@@ -521,24 +521,32 @@ export type LogRetentionDeleteResult = {
|
|||||||
blocked_targets?: BlockedTarget[];
|
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<RetentionHostsResult> {
|
export function listRetentionHosts(olderThanHours: number): Promise<RetentionHostsResult> {
|
||||||
return request(`/logs/retention/hosts?older_than_hours=${olderThanHours}`, { credentials: 'include' });
|
return request<RetentionHostsResult>(`/logs/retention/hosts?older_than_hours=${olderThanHours}`, {
|
||||||
|
credentials: 'include'
|
||||||
|
}).then((r) => ({ ...r, hosts: r.hosts ?? [] }));
|
||||||
}
|
}
|
||||||
|
|
||||||
export function previewLogDeletion(olderThanHours: number, targets: HostService[]): Promise<LogRetentionPreview> {
|
export function previewLogDeletion(olderThanHours: number, targets: HostService[]): Promise<LogRetentionPreview> {
|
||||||
return request('/logs/retention/preview', {
|
return request<LogRetentionPreview>('/logs/retention/preview', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
credentials: 'include',
|
credentials: 'include',
|
||||||
body: JSON.stringify({ older_than_hours: olderThanHours, targets })
|
body: JSON.stringify({ older_than_hours: olderThanHours, targets })
|
||||||
});
|
}).then((r) => ({ ...r, targets: r.targets ?? [] }));
|
||||||
}
|
}
|
||||||
|
|
||||||
export function deleteLogsOlderThan(olderThanHours: number, targets: HostService[]): Promise<LogRetentionDeleteResult> {
|
export function deleteLogsOlderThan(olderThanHours: number, targets: HostService[]): Promise<LogRetentionDeleteResult> {
|
||||||
return request('/logs/retention/delete', {
|
return request<LogRetentionDeleteResult>('/logs/retention/delete', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
credentials: 'include',
|
credentials: 'include',
|
||||||
body: JSON.stringify({ older_than_hours: olderThanHours, targets })
|
body: JSON.stringify({ older_than_hours: olderThanHours, targets })
|
||||||
});
|
}).then((r) => ({ ...r, deleted_targets: r.deleted_targets ?? [] }));
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- alerting ---------------------------------------------------------
|
// --- alerting ---------------------------------------------------------
|
||||||
|
|||||||
@@ -18,6 +18,7 @@
|
|||||||
} from '$lib/api';
|
} from '$lib/api';
|
||||||
import { getTheme, setTheme, type Theme } from '$lib/theme.svelte';
|
import { getTheme, setTheme, type Theme } from '$lib/theme.svelte';
|
||||||
import { getDensity, setDensity, type Density } from '$lib/density.svelte';
|
import { getDensity, setDensity, type Density } from '$lib/density.svelte';
|
||||||
|
import Skeleton from '$lib/components/ui/Skeleton.svelte';
|
||||||
|
|
||||||
let loading = $state(true);
|
let loading = $state(true);
|
||||||
let features = $state<AuthFeatures>({ sso_configured: false, oidc_enabled: false, saml_enabled: false });
|
let features = $state<AuthFeatures>({ sso_configured: false, oidc_enabled: false, saml_enabled: false });
|
||||||
@@ -122,7 +123,7 @@
|
|||||||
selectedTargets = new Set();
|
selectedTargets = new Set();
|
||||||
try {
|
try {
|
||||||
const result = await listRetentionHosts(hours);
|
const result = await listRetentionHosts(hours);
|
||||||
hosts = result.hosts;
|
hosts = result.hosts ?? [];
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
hostsError = e instanceof Error ? e.message : String(e);
|
hostsError = e instanceof Error ? e.message : String(e);
|
||||||
hosts = [];
|
hosts = [];
|
||||||
@@ -304,7 +305,12 @@
|
|||||||
{#if hostsError}<p class="error">{hostsError}</p>{/if}
|
{#if hostsError}<p class="error">{hostsError}</p>{/if}
|
||||||
|
|
||||||
{#if hostsLoading}
|
{#if hostsLoading}
|
||||||
<p class="muted">Loading hosts…</p>
|
<div class="host-picker-loading" aria-busy="true" aria-live="polite">
|
||||||
|
<span class="sr-only">Loading hosts…</span>
|
||||||
|
<Skeleton height="2.25rem" />
|
||||||
|
<Skeleton height="2.25rem" />
|
||||||
|
<Skeleton height="2.25rem" />
|
||||||
|
</div>
|
||||||
{:else if hosts.length === 0}
|
{:else if hosts.length === 0}
|
||||||
<p class="note">No hosts have logs older than this.</p>
|
<p class="note">No hosts have logs older than this.</p>
|
||||||
{:else}
|
{:else}
|
||||||
@@ -459,6 +465,18 @@
|
|||||||
.muted {
|
.muted {
|
||||||
color: var(--color-text-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 {
|
.note {
|
||||||
color: var(--color-text-muted);
|
color: var(--color-text-muted);
|
||||||
font-size: var(--text-sm);
|
font-size: var(--text-sm);
|
||||||
|
|||||||
Reference in New Issue
Block a user