perf(frontend): coalesce concurrent stack statuses fetches (#1816)

* perf(frontend): coalesce concurrent stack statuses fetches

Sidebar and dashboard both requested GET /stacks/statuses for the same
node; share one in-flight promise keyed by explicit nodeId so boot and
invalidate overlap stop doubling the hot path.

* fix(frontend): clear statuses coalescer by entry id

Avoid Promise identity checks that CodeQL flags as missing await.
Numeric entry ids still protect a newer in-flight fetch after logout clear.
This commit is contained in:
Anso
2026-08-10 14:57:17 -04:00
committed by GitHub
parent 27fe0ae837
commit 017ac57654
8 changed files with 375 additions and 47 deletions
@@ -22,6 +22,7 @@ vi.mock('@/hooks/useImageUpdates', () => ({
}));
import { useStackListState } from './useStackListState';
import { __resetStackStatusesFetchForTests } from '@/lib/stackStatusesFetch';
function okJson(payload: unknown): Response {
return new Response(JSON.stringify(payload), {
@@ -35,6 +36,7 @@ function notFound(): Response {
}
beforeEach(() => {
__resetStackStatusesFetchForTests();
apiFetchMock.mockReset();
useNodesMock.mockReset();
useImageUpdatesMock.mockReset();
@@ -1,5 +1,6 @@
import { useState, useEffect, useRef, useMemo, useCallback } from 'react';
import { apiFetch } from '@/lib/api';
import { fetchStackStatusesShared, type StackStatusesFetchResult } from '@/lib/stackStatusesFetch';
import {
newAttemptId,
abortAttempt,
@@ -297,27 +298,36 @@ export function useStackListState() {
// format can express `partial`; a node lacking the endpoint or returning
// the legacy plain-string format is re-derived from per-stack containers
// so a crashed container is not hidden behind a healthy sibling.
// Skip statuses until activeNode resolves (null here means unresolved, not
// "local"); never pass an unknown target into the shared fetch.
if (fetchNodeId === null) {
return fileList;
}
const statusHeaders = beginSpan('fetch_headers', { attemptId, background, proxied });
const statusRes = await apiFetch('/stacks/statuses');
const statusProxied = statusRes.headers.get('x-sencho-proxy') === '1' || proxied;
endSpan(statusHeaders, { proxied: statusProxied, detail: { status: statusRes.status } });
let statusResult: StackStatusesFetchResult;
try {
statusResult = await fetchStackStatusesShared(fetchNodeId);
} catch (statusErr) {
endSpan(statusHeaders, { outcome: 'error', detail: { coalesced: false } });
throw statusErr;
}
const statusProxied = statusResult.proxied || proxied;
// Joined waiters mark network spans superseded so truncated join timings
// are not mistaken for fast fetches.
endSpan(statusHeaders, {
outcome: statusResult.coalesced ? 'superseded' : undefined,
proxied: statusProxied,
detail: { status: statusResult.status, coalesced: statusResult.coalesced },
});
if (stale()) { abortAttempt(attemptId); return fileList; }
let bulkStatuses: Record<string, StackRowStatus> = {};
const bulkPorts: Record<string, number | undefined> = {};
const bulkSelf: Record<string, boolean> = {};
const bulkCounts: StackCounts = {};
let raw: unknown = null;
if (statusRes.ok) {
const statusBodySpan = beginSpan('body_decode', { attemptId, background, proxied: statusProxied });
try {
raw = await statusRes.json();
endSpan(statusBodySpan);
} catch (decodeErr) {
endSpan(statusBodySpan, { outcome: 'error' });
throw decodeErr;
}
}
// Decode already happened inside the shared helper; do not emit a fake
// body_decode span that would look like near-zero network work.
const raw: unknown = statusResult.ok ? statusResult.body : null;
if (isBulkStatusObjectFormat(raw)) {
for (const [key, val] of Object.entries(raw as Record<string, StackStatusInfo>)) {
bulkStatuses[key] = val.status;
@@ -330,7 +340,12 @@ export function useStackListState() {
} else {
bulkStatuses = await deriveStatusesFromContainers(fileList);
}
const statusDispatch = beginSpan('state_dispatch', { attemptId, background, proxied: statusProxied });
const statusDispatch = beginSpan('state_dispatch', {
attemptId,
background,
proxied: statusProxied,
detail: { coalesced: statusResult.coalesced },
});
setStackStatuses(prev => {
const next: StackStatus = {};
for (const file of fileList) {
@@ -346,7 +361,7 @@ export function useStackListState() {
});
setStackSelfFlags(bulkSelf);
setStackCounts(bulkCounts);
endSpan(statusDispatch);
endSpan(statusDispatch, { detail: { coalesced: statusResult.coalesced } });
refreshLabels();
if (!background) {
listHydratedPendingRef.current = { attemptId, token: listToken, proxied: statusProxied };
@@ -30,6 +30,7 @@ vi.mock('@/lib/utils', async () => {
});
import { useDashboardData } from '../useDashboardData';
import { __resetStackStatusesFetchForTests } from '@/lib/stackStatusesFetch';
function okJson(payload: unknown): Response {
return new Response(JSON.stringify(payload), {
@@ -91,6 +92,7 @@ async function tickSys(): Promise<void> {
beforeEach(() => {
pollCallbacks = [];
__resetStackStatusesFetchForTests();
apiFetchMock.mockReset();
useNodesMock.mockReset();
useNodesMock.mockReturnValue({
@@ -100,6 +102,7 @@ beforeEach(() => {
});
afterEach(() => {
__resetStackStatusesFetchForTests();
vi.clearAllMocks();
});
@@ -28,6 +28,7 @@ vi.mock('@/lib/utils', async () => {
});
import { useDashboardData } from '../useDashboardData';
import { __resetStackStatusesFetchForTests } from '@/lib/stackStatusesFetch';
function okJson(payload: unknown): Response {
return new Response(JSON.stringify(payload), {
@@ -49,6 +50,7 @@ const SYS_PAYLOAD = {
beforeEach(() => {
vi.useFakeTimers();
__resetStackStatusesFetchForTests();
apiFetchMock.mockReset();
apiFetchMock.mockImplementation((endpoint: string) => {
if (endpoint === '/stats') return Promise.resolve(okJson(STATS_PAYLOAD));
@@ -65,6 +67,7 @@ beforeEach(() => {
});
afterEach(() => {
__resetStackStatusesFetchForTests();
vi.useRealTimers();
});
@@ -169,7 +172,7 @@ describe('useDashboardData stackStatuses load states', () => {
expect(result.current.stackStatusesLoadStatus).toBe('success');
});
it('ignores an older soft success after a newer foreground retry', async () => {
it('joins a foreground retry onto an in-flight soft statuses fetch', async () => {
const resolvers: Array<(r: Response) => void> = [];
apiFetchMock.mockImplementation((endpoint: string) => {
if (endpoint === '/stats') return Promise.resolve(okJson(STATS_PAYLOAD));
@@ -200,27 +203,20 @@ describe('useDashboardData stackStatuses load states', () => {
result.current.retryStackStatuses();
await Promise.resolve();
});
expect(resolvers).toHaveLength(3);
// Soft and foreground share one in-flight /stacks/statuses.
expect(resolvers).toHaveLength(2);
const softMap = { 'old.yml': { status: 'exited' as const } };
const retryMap = { 'web.yml': { status: 'running' as const } };
const sharedMap = { 'web.yml': { status: 'running' as const } };
await act(async () => {
resolvers[1](okJson(softMap));
await Promise.resolve();
await Promise.resolve();
});
expect(result.current.stackStatuses).toEqual({});
await act(async () => {
resolvers[2](okJson(retryMap));
resolvers[1](okJson(sharedMap));
await Promise.resolve();
await Promise.resolve();
});
expect(result.current.stackStatusesLoadStatus).toBe('success');
expect(result.current.stackStatuses).toEqual(retryMap);
expect(result.current.stackStatuses).toEqual(sharedMap);
});
it('ignores an older soft failure after a newer foreground retry success', async () => {
it('lets the newer foreground generation commit a shared in-flight failure', async () => {
const resolvers: Array<(r: Response) => void> = [];
apiFetchMock.mockImplementation((endpoint: string) => {
if (endpoint === '/stats') return Promise.resolve(okJson(STATS_PAYLOAD));
@@ -237,7 +233,7 @@ describe('useDashboardData stackStatuses load states', () => {
expect(resolvers).toHaveLength(1);
await act(async () => {
resolvers[0](okJson({}));
resolvers[0](okJson({ 'web.yml': { status: 'running' } }));
await Promise.resolve();
await Promise.resolve();
});
@@ -250,23 +246,16 @@ describe('useDashboardData stackStatuses load states', () => {
result.current.retryStackStatuses();
await Promise.resolve();
});
expect(resolvers).toHaveLength(3);
const retryMap = { 'web.yml': { status: 'running' as const } };
await act(async () => {
resolvers[2](okJson(retryMap));
await Promise.resolve();
await Promise.resolve();
});
expect(result.current.stackStatuses).toEqual(retryMap);
expect(resolvers).toHaveLength(2);
await act(async () => {
resolvers[1](new Response('nope', { status: 500 }));
await Promise.resolve();
await Promise.resolve();
});
expect(result.current.stackStatusesLoadStatus).toBe('success');
expect(result.current.stackStatuses).toEqual(retryMap);
// Soft's older generation is ignored; the foreground generation commits
// the shared failure as an error status (rows are not cleared on failure).
expect(result.current.stackStatusesLoadStatus).toBe('error');
});
it('lets a slow foreground statuses response commit after soft poll and invalidate ticks', async () => {
@@ -382,4 +371,30 @@ describe('useDashboardData stackStatuses load states', () => {
expect(result.current.stackStatusesLoadStatus).toBe('error');
expect(result.current.stackStatuses).toEqual({});
});
it('does not fetch statuses while activeNode is unresolved and stays loading', async () => {
useNodesMock.mockReturnValue({
activeNode: undefined,
nodes: [],
});
const { result, rerender } = renderHook(() => useDashboardData());
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
expect(countFetchCalls('/stacks/statuses')).toBe(0);
expect(result.current.stackStatusesLoadStatus).toBe('loading');
expect(result.current.stackStatuses).toEqual({});
useNodesMock.mockReturnValue({
activeNode: { id: 7, name: 'Remote', type: 'remote' },
nodes: [{ id: 7, name: 'Remote', type: 'remote' }],
});
rerender();
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
expect(countFetchCalls('/stacks/statuses')).toBe(1);
expect(apiFetchMock.mock.calls.some(
(call) => call[0] === '/stacks/statuses' && call[1]?.nodeId === 7,
)).toBe(true);
});
});
@@ -1,6 +1,7 @@
import { useState, useEffect, useCallback, useMemo, useRef } from 'react';
import { useNodes } from '@/context/NodeContext';
import { apiFetch } from '@/lib/api';
import { fetchStackStatusesShared } from '@/lib/stackStatusesFetch';
import { visibilityInterval } from '@/lib/utils';
import type {
Stats,
@@ -270,6 +271,10 @@ export function useDashboardData(): DashboardData {
currentNodeId: number | undefined,
mode: 'foreground' | 'soft',
) => {
// Skip until activeNode resolves. Passing null/"local" here would fetch the
// hub while a remembered remote node is still loading and commit under an
// unresolved guard.
if (currentNodeId === undefined) return;
if (nodeIdRef.current !== currentNodeId) return;
if (mode === 'soft' && stackStatusesInFlightRef.current) return;
const generation = ++stackStatusesFetchGenRef.current;
@@ -279,19 +284,18 @@ export function useDashboardData(): DashboardData {
setStackStatusesLoadError(null);
}
try {
const res = await apiFetch('/stacks/statuses');
const result = await fetchStackStatusesShared(currentNodeId);
if (!isCurrentStatusesFetch(currentNodeId, generation)) return;
if (!res.ok) {
if (!result.ok) {
commitStackStatusesFailure(
currentNodeId,
generation,
mode,
`Could not load stack health (${res.status}).`,
`Could not load stack health (${result.status}).`,
);
return;
}
const body: unknown = await res.json();
if (!isCurrentStatusesFetch(currentNodeId, generation)) return;
const body = result.body;
if (body && typeof body === 'object' && !Array.isArray(body)) {
// Drop any entry isValidStatusEntry rejects rather than trusting the
// whole map: one bad entry must not crash or misrepresent the rest of
@@ -352,6 +356,9 @@ export function useDashboardData(): DashboardData {
setStackStatusesLoadStatus('loading');
setStackStatusesLoadError(null);
const currentNodeId = nodeId;
// Stay in loading while NodeContext has not resolved activeNode; do not
// fetch against an unknown target (and do not treat that window as empty).
if (currentNodeId === undefined) return;
void fetchStackStatuses(currentNodeId, 'foreground');
const cleanup = visibilityInterval(() => {
void fetchStackStatuses(currentNodeId, 'soft');
@@ -370,6 +377,7 @@ export function useDashboardData(): DashboardData {
// refresh instead of one HTTP request per event.
useEffect(() => {
const currentNodeId = nodeId;
if (currentNodeId === undefined) return;
let active = true;
let invalidateTimer: ReturnType<typeof setTimeout> | null = null;
const refresh = async () => {
+4
View File
@@ -1,5 +1,6 @@
import { createContext, useContext, useState, useEffect, useCallback, useRef, type ReactNode } from 'react';
import { markMilestone } from '@/lib/hydrationTiming';
import { clearStackStatusesFetch } from '@/lib/stackStatusesFetch';
import { resolveCan } from '@/lib/resolveCan';
type AppStatus = 'loading' | 'needsSetup' | 'notAuthenticated' | 'mfaChallenge' | 'authenticated';
@@ -132,6 +133,7 @@ export function AuthProvider({ children }: { children: ReactNode }) {
useEffect(() => {
checkAuth();
const handleUnauthorized = () => {
clearStackStatusesFetch();
setUser(null);
resetPermissions();
setAppStatus('notAuthenticated');
@@ -235,6 +237,7 @@ export function AuthProvider({ children }: { children: ReactNode }) {
} catch (error) {
console.error('Cancel MFA error:', error);
} finally {
clearStackStatusesFetch();
setUser(null);
resetPermissions();
setAppStatus('notAuthenticated');
@@ -250,6 +253,7 @@ export function AuthProvider({ children }: { children: ReactNode }) {
} catch (error) {
console.error('Logout error:', error);
} finally {
clearStackStatusesFetch();
setUser(null);
resetPermissions();
setAppStatus('notAuthenticated');
@@ -0,0 +1,191 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import * as api from '@/lib/api';
import {
__resetStackStatusesFetchForTests,
clearStackStatusesFetch,
fetchStackStatusesShared,
} from '@/lib/stackStatusesFetch';
function jsonResponse(body: unknown, init: { status?: number; proxied?: boolean } = {}): Response {
const headers = new Headers({ 'Content-Type': 'application/json' });
if (init.proxied) headers.set('x-sencho-proxy', '1');
return new Response(JSON.stringify(body), {
status: init.status ?? 200,
headers,
});
}
function deferredResponse(): {
promise: Promise<Response>;
release: (res: Response) => void;
} {
let release!: (res: Response) => void;
const promise = new Promise<Response>((resolve) => {
release = resolve;
});
return { promise, release };
}
describe('fetchStackStatusesShared', () => {
let apiFetchSpy: ReturnType<typeof vi.spyOn>;
beforeEach(() => {
__resetStackStatusesFetchForTests();
apiFetchSpy = vi.spyOn(api, 'apiFetch');
});
afterEach(() => {
__resetStackStatusesFetchForTests();
apiFetchSpy.mockRestore();
});
it('coalesces concurrent callers for the same nodeId into one apiFetch', async () => {
const gate = deferredResponse();
apiFetchSpy.mockReturnValueOnce(gate.promise);
const a = fetchStackStatusesShared(1);
const b = fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(1);
expect(apiFetchSpy).toHaveBeenCalledWith('/stacks/statuses', { nodeId: 1 });
gate.release(jsonResponse({ 'demo.yml': { status: 'running' } }));
const [ra, rb] = await Promise.all([a, b]);
expect(ra.coalesced).toBe(false);
expect(rb.coalesced).toBe(true);
expect(ra.body).toEqual(rb.body);
expect(ra.ok).toBe(true);
});
it('issues a second fetch when the second caller starts after the first resolves', async () => {
apiFetchSpy
.mockResolvedValueOnce(jsonResponse({ a: { status: 'running' } }))
.mockResolvedValueOnce(jsonResponse({ b: { status: 'exited' } }));
const first = await fetchStackStatusesShared(1);
const second = await fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
expect(first.coalesced).toBe(false);
expect(second.coalesced).toBe(false);
expect(first.body).not.toEqual(second.body);
});
it('does not share across different nodeIds', async () => {
apiFetchSpy
.mockResolvedValueOnce(jsonResponse({ local: { status: 'running' } }))
.mockResolvedValueOnce(jsonResponse({ remote: { status: 'running' } }));
const [a, b] = await Promise.all([
fetchStackStatusesShared(1),
fetchStackStatusesShared(2),
]);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
expect(a.coalesced).toBe(false);
expect(b.coalesced).toBe(false);
});
it('forwards explicit null as the local key and apiFetch nodeId', async () => {
apiFetchSpy.mockResolvedValueOnce(jsonResponse({}));
await fetchStackStatusesShared(null);
expect(apiFetchSpy).toHaveBeenCalledWith('/stacks/statuses', { nodeId: null });
});
it('ignores localStorage divergence when an explicit nodeId is passed', async () => {
localStorage.setItem('sencho-active-node', '99');
const gate = deferredResponse();
apiFetchSpy.mockReturnValueOnce(gate.promise);
const a = fetchStackStatusesShared(3);
const b = fetchStackStatusesShared(3);
expect(apiFetchSpy).toHaveBeenCalledTimes(1);
expect(apiFetchSpy).toHaveBeenCalledWith('/stacks/statuses', { nodeId: 3 });
gate.release(jsonResponse({}));
await Promise.all([a, b]);
localStorage.removeItem('sencho-active-node');
});
it('clears on sencho-unauthorized so the next caller issues a fresh fetch', async () => {
const gate = deferredResponse();
apiFetchSpy
.mockReturnValueOnce(gate.promise)
.mockResolvedValueOnce(jsonResponse({ after: { status: 'exited' } }));
const pending = fetchStackStatusesShared(1);
window.dispatchEvent(new Event('sencho-unauthorized'));
gate.release(jsonResponse({ before: { status: 'running' } }));
await pending;
const next = await fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
expect(next.coalesced).toBe(false);
expect((next.body as Record<string, unknown>).after).toBeTruthy();
});
it('clearStackStatusesFetch clears without requiring the window event', async () => {
const gate = deferredResponse();
apiFetchSpy
.mockReturnValueOnce(gate.promise)
.mockResolvedValueOnce(jsonResponse({}));
const pending = fetchStackStatusesShared(1);
clearStackStatusesFetch();
gate.release(jsonResponse({ stale: true }));
await pending;
await fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
});
it('stale settlement after clear does not delete a newer in-flight entry', async () => {
const staleGate = deferredResponse();
const freshGate = deferredResponse();
apiFetchSpy
.mockReturnValueOnce(staleGate.promise)
.mockReturnValueOnce(freshGate.promise);
const stale = fetchStackStatusesShared(1);
clearStackStatusesFetch();
const fresh = fetchStackStatusesShared(1);
const joined = fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
staleGate.release(jsonResponse({ stale: true }));
await stale;
// Fresh slot must still be joinable after the stale owner settles.
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
freshGate.release(jsonResponse({ fresh: { status: 'running' } }));
const [freshResult, joinedResult] = await Promise.all([fresh, joined]);
expect(freshResult.coalesced).toBe(false);
expect(joinedResult.coalesced).toBe(true);
expect(freshResult.body).toEqual(joinedResult.body);
});
it('does not retain a rejected promise for later callers', async () => {
apiFetchSpy
.mockRejectedValueOnce(new Error('network down'))
.mockResolvedValueOnce(jsonResponse({ ok: { status: 'running' } }));
await expect(fetchStackStatusesShared(1)).rejects.toThrow('network down');
const recovered = await fetchStackStatusesShared(1);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
expect(recovered.ok).toBe(true);
});
it('propagates JSON decode failures to both waiters and clears the slot', async () => {
const bad = new Response('not-json', {
status: 200,
headers: { 'Content-Type': 'application/json' },
});
apiFetchSpy
.mockResolvedValueOnce(bad)
.mockResolvedValueOnce(jsonResponse({}));
const a = fetchStackStatusesShared(1);
const b = fetchStackStatusesShared(1);
await expect(a).rejects.toThrow();
await expect(b).rejects.toThrow();
const recovered = await fetchStackStatusesShared(1);
expect(recovered.ok).toBe(true);
expect(apiFetchSpy).toHaveBeenCalledTimes(2);
});
});
+90
View File
@@ -0,0 +1,90 @@
import { apiFetch } from '@/lib/api';
/**
* Raw transport result for GET /stacks/statuses. Callers keep all
* interpretation (sanitization, legacy-format fallback, error copy).
*/
export type StackStatusesFetchResult = {
ok: boolean;
status: number;
proxied: boolean;
body: unknown;
/** True when this waiter joined an in-flight promise owned by another caller. */
coalesced: boolean;
};
type NodeKey = string;
type InflightEntry = {
/** Monotonic id so settled owners clear only their own map slot. */
id: number;
promise: Promise<StackStatusesFetchResult>;
};
const inflight = new Map<NodeKey, InflightEntry>();
let nextInflightId = 0;
function nodeKey(nodeId: number | null): NodeKey {
return nodeId === null ? 'local' : String(nodeId);
}
async function requestStackStatuses(
nodeId: number | null,
): Promise<StackStatusesFetchResult> {
const res = await apiFetch('/stacks/statuses', { nodeId });
const proxied = res.headers.get('x-sencho-proxy') === '1';
let body: unknown = null;
if (res.ok) {
body = await res.json();
}
return {
ok: res.ok,
status: res.status,
proxied,
body,
coalesced: false,
};
}
/** Drop every in-flight join so a prior auth session cannot share results. */
export function clearStackStatusesFetch(): void {
inflight.clear();
}
if (typeof window !== 'undefined') {
window.addEventListener('sencho-unauthorized', clearStackStatusesFetch);
}
/**
* Coalesce concurrent GET /stacks/statuses for the same node/auth context.
* Always forwards an explicit nodeId to apiFetch so the key and request target
* cannot diverge from localStorage during a node switch.
*/
export async function fetchStackStatusesShared(
nodeId: number | null,
): Promise<StackStatusesFetchResult> {
const key = nodeKey(nodeId);
const existing = inflight.get(key);
if (existing) {
const shared = await existing.promise;
return { ...shared, coalesced: true };
}
const id = ++nextInflightId;
const promise = requestStackStatuses(nodeId);
inflight.set(key, { id, promise });
try {
return await promise;
} finally {
// Clear only if this owner still holds the slot. A logout clear followed by
// a new fetch must not be deleted by a stale settlement.
if (inflight.get(key)?.id === id) {
inflight.delete(key);
}
}
}
/** Test-only: reset module state between vitest cases. */
export function __resetStackStatusesFetchForTests(): void {
clearStackStatusesFetch();
}