mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-04 07:57:54 +00:00
fix(resources): harden Resources Hub data race, prune errors, and scan lifecycle (#1251)
* fix(resources): harden Resources Hub data race, prune errors, and scan lifecycle Guard fetchAllData with a generation counter so switching nodes mid-load cannot let a stale fetch overwrite the newly selected node's resources. handlePrune now checks res.ok and surfaces the server error instead of showing a false success toast on a failed prune, matching the delete and purge handlers. Make the vulnerability-scan poll loop abort-aware via an AbortController that cancels on unmount and on node switch, dismissing the loading toast and avoiding state updates after the view is gone. Add developer-mode diagnostic logging to the prune, network-create, and volume list/read paths, gated by the existing developer_mode setting and never logging file bodies or secrets. Add regression tests covering the node-switch race and the prune error path. * fix(resources): make scan-poll abort cancel in-flight requests and guard parse window Thread the scan AbortController signal into the scan POST, status poll, and image-summaries fetches so an abort cancels the in-flight request and the loading toast clears promptly instead of after the request settles. Add abort checks after each response-body parse so an abort landing during JSON parsing cannot fire a stale completion toast, open the scan sheet, or write summaries for a node the user already left. Bump the fetch generation on unmount so a fetchAllData that resolves after the view is gone drops its state writes and load-error toast. Add a regression test for the post-unmount load-failure path.
This commit is contained in:
@@ -120,11 +120,17 @@ systemMaintenanceRouter.post('/prune/system', async (req: Request, res: Response
|
||||
'docker disk usage',
|
||||
);
|
||||
}
|
||||
if (isDebugEnabled()) {
|
||||
console.debug('[Resources:debug] Prune dry-run', {
|
||||
target, scope: pruneScope, reclaimableBytes: estimate.reclaimableBytes,
|
||||
});
|
||||
}
|
||||
res.json({ message: 'Dry run', success: true, dryRun: true, reclaimedBytes: estimate.reclaimableBytes });
|
||||
return;
|
||||
}
|
||||
|
||||
console.log(`[Resources] System prune: ${target} (scope: ${pruneScope})`);
|
||||
const pruneStartedAt = Date.now();
|
||||
let result: { success: boolean; reclaimedBytes: number };
|
||||
if (pruneScope === 'managed' && target !== 'containers') {
|
||||
const knownStacks = await FileSystemService.getInstance(req.nodeId).getStacks();
|
||||
@@ -137,6 +143,11 @@ systemMaintenanceRouter.post('/prune/system', async (req: Request, res: Response
|
||||
}
|
||||
|
||||
console.log(`[Resources] System prune completed: ${target}, reclaimed ${result.reclaimedBytes} bytes`);
|
||||
if (isDebugEnabled()) {
|
||||
console.debug('[Resources:debug] System prune', {
|
||||
target, scope: pruneScope, ms: Date.now() - pruneStartedAt, reclaimedBytes: result.reclaimedBytes,
|
||||
});
|
||||
}
|
||||
if (target === 'containers') {
|
||||
invalidateNodeCaches(req.nodeId);
|
||||
}
|
||||
@@ -390,6 +401,15 @@ systemMaintenanceRouter.post('/networks', async (req: Request, res: Response) =>
|
||||
if (attachable) options.Attachable = true;
|
||||
|
||||
const dockerController = DockerController.getInstance(req.nodeId);
|
||||
if (isDebugEnabled()) {
|
||||
console.debug('[Resources:debug] Network create', {
|
||||
driver: options.Driver ?? 'bridge',
|
||||
internal: !!options.Internal,
|
||||
attachable: !!options.Attachable,
|
||||
hasSubnet: !!subnet,
|
||||
hasGateway: !!gateway,
|
||||
});
|
||||
}
|
||||
const network = await dockerController.createNetwork(options);
|
||||
console.log(`[Resources] Network created: ${sanitizeForLog(name)}`);
|
||||
invalidateNodeCaches(req.nodeId);
|
||||
|
||||
@@ -3,6 +3,7 @@ import { VolumeBrowserService, isValidVolumeName, PathTraversalError, VolumeNotF
|
||||
import { DatabaseService } from '../services/DatabaseService';
|
||||
import { requireAdmin } from '../middleware/tierGates';
|
||||
import { sanitizeForLog } from '../utils/safeLog';
|
||||
import { isDebugEnabled } from '../utils/debug';
|
||||
|
||||
export const volumesRouter = Router();
|
||||
|
||||
@@ -27,7 +28,13 @@ volumesRouter.get('/:name/list', async (req: Request, res: Response) => {
|
||||
const name = req.params.name as string;
|
||||
if (!isValidVolumeName(name)) return res.status(400).json({ error: 'Invalid volume name' });
|
||||
const path = readPathParam(req);
|
||||
const startedAt = Date.now();
|
||||
const entries = await VolumeBrowserService.getInstance(req.nodeId).listDir(name, path);
|
||||
if (isDebugEnabled()) {
|
||||
console.debug('[Volumes:debug] list', {
|
||||
volume: sanitizeForLog(name), path: sanitizeForLog(path || '/'), entries: entries.length, ms: Date.now() - startedAt,
|
||||
});
|
||||
}
|
||||
res.json(entries);
|
||||
} catch (error: unknown) {
|
||||
mapServiceError(error, res, 'Failed to list volume directory');
|
||||
@@ -54,8 +61,16 @@ volumesRouter.get('/:name/read', async (req: Request, res: Response) => {
|
||||
let outcome: 'success' | 'error' = 'error';
|
||||
try {
|
||||
if (!isValidVolumeName(name)) return res.status(400).json({ error: 'Invalid volume name' });
|
||||
const startedAt = Date.now();
|
||||
const result = await VolumeBrowserService.getInstance(req.nodeId).readFile(name, requestPath);
|
||||
outcome = 'success';
|
||||
if (isDebugEnabled()) {
|
||||
// Never log the file body; only its shape.
|
||||
console.debug('[Volumes:debug] read', {
|
||||
volume: sanitizeForLog(name), path: sanitizeForLog(requestPath || '/'),
|
||||
size: result.size, binary: result.binary, truncated: result.truncated, ms: Date.now() - startedAt,
|
||||
});
|
||||
}
|
||||
res.json(result);
|
||||
} catch (error: unknown) {
|
||||
mapServiceError(error, res, 'Failed to read volume file');
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useState, useEffect } from 'react';
|
||||
import { useState, useEffect, useRef } from 'react';
|
||||
import { Card, CardContent, CardHeader, CardTitle, CardDescription } from "@/components/ui/card";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Tabs, TabsContent, TabsList, TabsTrigger, TabsHighlight, TabsHighlightItem } from "@/components/ui/tabs";
|
||||
@@ -386,8 +386,17 @@ export default function ResourcesView() {
|
||||
const [scanSummaries, setScanSummaries] = useState<Record<string, ScanSummary>>({});
|
||||
const [scanningImageRef, setScanningImageRef] = useState<string | null>(null);
|
||||
const [inspectScanId, setInspectScanId] = useState<number | null>(null);
|
||||
// Holds the AbortController for the in-flight scan poll so it can be
|
||||
// cancelled; the scan keeps running server-side, only the client poll stops.
|
||||
const scanAbortRef = useRef<AbortController | null>(null);
|
||||
|
||||
// Generation counter so a slow fetch for a previously-active node cannot
|
||||
// stomp the visible resources after the user switches nodes. Each call
|
||||
// claims a generation; only the latest call is allowed to write state.
|
||||
const fetchGenerationRef = useRef(0);
|
||||
|
||||
const fetchAllData = async () => {
|
||||
const generation = ++fetchGenerationRef.current;
|
||||
setIsLoading(true);
|
||||
try {
|
||||
const [usageRes, resourcesRes, orphansRes, summariesRes] = await Promise.all([
|
||||
@@ -397,31 +406,47 @@ export default function ResourcesView() {
|
||||
apiFetch('/security/image-summaries').catch(() => null),
|
||||
]);
|
||||
|
||||
if (usageRes.ok) setUsage(await usageRes.json());
|
||||
if (resourcesRes.ok) {
|
||||
const resources = await resourcesRes.json();
|
||||
setImages(resources.images ?? []);
|
||||
setVolumes(resources.volumes ?? []);
|
||||
setNetworks(resources.networks ?? []);
|
||||
// Resolve every body before the staleness check so a stale
|
||||
// generation cannot write any subset of the resource slices.
|
||||
const usageData = usageRes.ok ? await usageRes.json() : null;
|
||||
const resourcesData = resourcesRes.ok ? await resourcesRes.json() : null;
|
||||
const orphansData = orphansRes.ok ? await orphansRes.json() : null;
|
||||
const summariesData = summariesRes && summariesRes.ok ? await summariesRes.json() : null;
|
||||
|
||||
if (fetchGenerationRef.current !== generation) return;
|
||||
|
||||
if (usageData) setUsage(usageData);
|
||||
if (resourcesData) {
|
||||
setImages(resourcesData.images ?? []);
|
||||
setVolumes(resourcesData.volumes ?? []);
|
||||
setNetworks(resourcesData.networks ?? []);
|
||||
}
|
||||
if (orphansRes.ok) {
|
||||
setOrphans(await orphansRes.json());
|
||||
if (orphansData) {
|
||||
setOrphans(orphansData);
|
||||
setSelectedOrphans([]);
|
||||
}
|
||||
if (summariesRes && summariesRes.ok) {
|
||||
const data = await summariesRes.json();
|
||||
setScanSummaries(data ?? {});
|
||||
}
|
||||
if (summariesData) setScanSummaries(summariesData);
|
||||
} catch (err) {
|
||||
if (fetchGenerationRef.current !== generation) return;
|
||||
console.error('Failed to fetch data', err);
|
||||
toast.error('Failed to load resources data');
|
||||
} finally {
|
||||
setIsLoading(false);
|
||||
if (fetchGenerationRef.current === generation) setIsLoading(false);
|
||||
}
|
||||
};
|
||||
|
||||
useEffect(() => { fetchAllData(); }, [activeNode]);
|
||||
|
||||
// Cancel an in-flight scan poll on unmount or node switch; its result
|
||||
// belongs to the node it started on.
|
||||
useEffect(() => {
|
||||
return () => scanAbortRef.current?.abort();
|
||||
}, [activeNode]);
|
||||
|
||||
// Bump the fetch generation on unmount so a fetch that resolves after the
|
||||
// view is gone cannot run state setters or surface a load-error toast.
|
||||
useEffect(() => () => { fetchGenerationRef.current += 1; }, []);
|
||||
|
||||
const handlePrune = async () => {
|
||||
if (!confirmPrune) return;
|
||||
setIsActioning(true);
|
||||
@@ -431,16 +456,21 @@ export default function ResourcesView() {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ target: confirmPrune.target, scope: confirmPrune.scope })
|
||||
});
|
||||
const data = await res.json();
|
||||
const data = await res.json().catch(() => null);
|
||||
if (!res.ok) {
|
||||
throw new Error(data?.error || `Failed to prune ${confirmPrune.target}`);
|
||||
}
|
||||
const scopeLabel = confirmPrune.scope === 'managed' ? 'Sencho-managed' : 'all';
|
||||
toast.success(
|
||||
data.reclaimedBytes !== undefined
|
||||
data?.reclaimedBytes !== undefined
|
||||
? `Pruned ${scopeLabel} ${confirmPrune.target}. Reclaimed ${formatBytes(data.reclaimedBytes)}.`
|
||||
: `Pruned ${scopeLabel} ${confirmPrune.target}.`
|
||||
);
|
||||
await fetchAllData();
|
||||
} catch {
|
||||
toast.error(confirmPrune ? `Failed to prune ${confirmPrune.target}` : 'Prune failed');
|
||||
} catch (error) {
|
||||
console.error('Failed to prune', error);
|
||||
const err = error as { message?: string };
|
||||
toast.error(err?.message || `Failed to prune ${confirmPrune.target}`);
|
||||
} finally {
|
||||
toast.dismiss(loadingId);
|
||||
setIsActioning(false);
|
||||
@@ -511,12 +541,18 @@ export default function ResourcesView() {
|
||||
options: { force?: boolean; scanners?: ('vuln' | 'secret')[] } = {},
|
||||
) => {
|
||||
const { force = false, scanners } = options;
|
||||
// Supersede any prior in-flight scan poll before claiming this one.
|
||||
scanAbortRef.current?.abort();
|
||||
const controller = new AbortController();
|
||||
scanAbortRef.current = controller;
|
||||
const { signal } = controller;
|
||||
setScanningImageRef(imageRef);
|
||||
const loadingId = toast.loading(`Scanning ${imageRef}...`);
|
||||
try {
|
||||
const res = await apiFetch('/security/scan', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ imageRef, force, scanners }),
|
||||
signal,
|
||||
});
|
||||
const data = await res.json();
|
||||
if (!res.ok) throw new Error(data?.error || 'Failed to start scan');
|
||||
@@ -524,19 +560,28 @@ export default function ResourcesView() {
|
||||
|
||||
const deadline = Date.now() + 5 * 60 * 1000;
|
||||
while (Date.now() < deadline) {
|
||||
await new Promise(r => setTimeout(r, 3000));
|
||||
const poll = await apiFetch(`/security/scans/${scanId}`);
|
||||
await new Promise<void>((resolve) => {
|
||||
if (signal.aborted) { resolve(); return; }
|
||||
const timer = setTimeout(resolve, 3000);
|
||||
signal.addEventListener('abort', () => { clearTimeout(timer); resolve(); }, { once: true });
|
||||
});
|
||||
if (signal.aborted) return;
|
||||
const poll = await apiFetch(`/security/scans/${scanId}`, { signal });
|
||||
if (signal.aborted) return;
|
||||
if (!poll.ok) continue;
|
||||
const poll_data = await poll.json();
|
||||
if (signal.aborted) return;
|
||||
if (poll_data.status !== 'in_progress') {
|
||||
if (poll_data.status === 'failed') {
|
||||
throw new Error(poll_data.error || 'Scan failed');
|
||||
}
|
||||
toast.success(`Scan complete: ${poll_data.total_vulnerabilities} vulnerabilities found`);
|
||||
setInspectScanId(scanId);
|
||||
const summariesRes = await apiFetch('/security/image-summaries');
|
||||
const summariesRes = await apiFetch('/security/image-summaries', { signal });
|
||||
if (signal.aborted) return;
|
||||
if (summariesRes.ok) {
|
||||
const summaries = await summariesRes.json();
|
||||
if (signal.aborted) return;
|
||||
setScanSummaries(summaries ?? {});
|
||||
}
|
||||
return;
|
||||
@@ -544,11 +589,22 @@ export default function ResourcesView() {
|
||||
}
|
||||
throw new Error('Scan timed out');
|
||||
} catch (error) {
|
||||
if (signal.aborted) {
|
||||
// Suppress the toast for a deliberately cancelled poll, but keep
|
||||
// a breadcrumb so a real error racing the abort is not lost.
|
||||
console.debug('Scan poll aborted', error);
|
||||
return;
|
||||
}
|
||||
const err = error as { message?: string; error?: string; data?: { error?: string } };
|
||||
toast.error(err?.message || err?.error || err?.data?.error || 'Scan failed');
|
||||
} finally {
|
||||
toast.dismiss(loadingId);
|
||||
setScanningImageRef(null);
|
||||
// Only the owning poll resets shared state; a superseded poll leaves
|
||||
// it to the scan that replaced it.
|
||||
if (scanAbortRef.current === controller) {
|
||||
scanAbortRef.current = null;
|
||||
setScanningImageRef(null);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -0,0 +1,163 @@
|
||||
/**
|
||||
* Coverage for ResourcesView hardening.
|
||||
*
|
||||
* Locks two correctness fixes that manual smoke testing cannot reliably catch:
|
||||
* - M-1: a slow resource fetch for a previously-active node must not overwrite
|
||||
* the newly-selected node's data (node-switch generation guard).
|
||||
* - M-2: a failed prune must surface the server error, never a false success.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
import { render, screen, waitFor } from '@testing-library/react';
|
||||
import userEvent from '@testing-library/user-event';
|
||||
|
||||
vi.mock('@/lib/api', () => ({ apiFetch: vi.fn() }));
|
||||
|
||||
vi.mock('@/components/ui/toast-store', () => ({
|
||||
toast: {
|
||||
error: vi.fn(),
|
||||
success: vi.fn(),
|
||||
warning: vi.fn(),
|
||||
info: vi.fn(),
|
||||
loading: vi.fn(() => 'toast-id'),
|
||||
dismiss: vi.fn(),
|
||||
},
|
||||
}));
|
||||
|
||||
const licenseState = { isPaid: true };
|
||||
vi.mock('@/context/LicenseContext', () => ({ useLicense: () => licenseState }));
|
||||
vi.mock('@/context/AuthContext', () => ({ useAuth: () => ({ isAdmin: true }) }));
|
||||
|
||||
const nodesState: { activeNode: { id: number } | null } = { activeNode: { id: 1 } };
|
||||
vi.mock('@/context/NodeContext', () => ({ useNodes: () => nodesState }));
|
||||
|
||||
vi.mock('@/hooks/useTrivyStatus', () => ({
|
||||
useTrivyStatus: () => ({
|
||||
status: { available: false, version: null, source: 'none', autoUpdate: false, busy: false },
|
||||
updateCheck: null,
|
||||
refresh: vi.fn(),
|
||||
refreshUpdateCheck: vi.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
// Heavy or portal-bound children are not under test; stub them to keep the
|
||||
// render tree light and deterministic.
|
||||
vi.mock('../VulnerabilityScanSheet', () => ({ VulnerabilityScanSheet: () => null }));
|
||||
vi.mock('../resources/ReclaimHero', () => ({ ReclaimHero: () => null }));
|
||||
vi.mock('../resources/FootprintTreemap', () => ({ FootprintTreemap: () => null }));
|
||||
vi.mock('../resources/ImageDetailsSheet', () => ({ ImageDetailsSheet: () => null }));
|
||||
vi.mock('../resources/VolumeBrowserSheet', () => ({ VolumeBrowserSheet: () => null }));
|
||||
vi.mock('../resources/NetworkDetailSheet', () => ({ NetworkDetailSheet: () => null }));
|
||||
vi.mock('../NetworkTopologyView', () => ({ default: () => null }));
|
||||
vi.mock('../CapabilityGate', () => ({ CapabilityGate: ({ children }: { children: React.ReactNode }) => <>{children}</> }));
|
||||
vi.mock('../LazyBoundary', () => ({ default: ({ children }: { children: React.ReactNode }) => <>{children}</> }));
|
||||
vi.mock('../NodeManager', () => ({ SENCHO_NAVIGATE_EVENT: 'sencho-navigate' }));
|
||||
|
||||
import { apiFetch } from '@/lib/api';
|
||||
import { toast } from '@/components/ui/toast-store';
|
||||
import ResourcesView from '../ResourcesView';
|
||||
|
||||
const mockedFetch = apiFetch as unknown as ReturnType<typeof vi.fn>;
|
||||
|
||||
function jsonResponse(body: unknown, init: { ok?: boolean; status?: number } = {}): Response {
|
||||
return {
|
||||
ok: init.ok ?? true,
|
||||
status: init.status ?? 200,
|
||||
json: async () => body,
|
||||
} as unknown as Response;
|
||||
}
|
||||
|
||||
function image(repoTag: string) {
|
||||
return {
|
||||
Id: `sha256:${repoTag}`,
|
||||
RepoTags: [repoTag],
|
||||
Size: 1000,
|
||||
Containers: 0,
|
||||
managedBy: null,
|
||||
managedStatus: 'unmanaged' as const,
|
||||
isSencho: false,
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
mockedFetch.mockReset();
|
||||
licenseState.isPaid = true;
|
||||
nodesState.activeNode = { id: 1 };
|
||||
});
|
||||
|
||||
afterEach(() => vi.clearAllMocks());
|
||||
|
||||
describe('ResourcesView', () => {
|
||||
it('drops a stale node fetch so it cannot overwrite the newly selected node (M-1)', async () => {
|
||||
// Hold every /system/resources response open so we control resolution order.
|
||||
const resourcesResolvers: Array<(r: Response) => void> = [];
|
||||
mockedFetch.mockImplementation((url: string) => {
|
||||
if (url === '/system/resources') {
|
||||
return new Promise<Response>((resolve) => resourcesResolvers.push(resolve));
|
||||
}
|
||||
return Promise.resolve(jsonResponse({}));
|
||||
});
|
||||
|
||||
const { rerender } = render(<ResourcesView />);
|
||||
await waitFor(() => expect(resourcesResolvers).toHaveLength(1));
|
||||
|
||||
// Switch nodes before the first fetch resolves; the effect re-runs and
|
||||
// claims a newer generation.
|
||||
nodesState.activeNode = { id: 2 };
|
||||
rerender(<ResourcesView />);
|
||||
await waitFor(() => expect(resourcesResolvers).toHaveLength(2));
|
||||
|
||||
// Resolve the newer (node 2) fetch first; its data should render.
|
||||
resourcesResolvers[1](jsonResponse({ images: [image('node2-img:latest')], volumes: [], networks: [] }));
|
||||
expect(await screen.findByText('node2-img:latest')).toBeInTheDocument();
|
||||
|
||||
// Now resolve the stale (node 1) fetch. Its generation is old, so the guard
|
||||
// must drop it rather than stomp node 2's resources.
|
||||
resourcesResolvers[0](jsonResponse({ images: [image('node1-img:latest')], volumes: [], networks: [] }));
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('node2-img:latest')).toBeInTheDocument();
|
||||
});
|
||||
expect(screen.queryByText('node1-img:latest')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('does not surface a load failure that resolves after the view unmounts', async () => {
|
||||
let rejectResources: ((e: unknown) => void) | undefined;
|
||||
mockedFetch.mockImplementation((url: string) => {
|
||||
if (url === '/system/resources') {
|
||||
return new Promise<Response>((_resolve, reject) => { rejectResources = reject; });
|
||||
}
|
||||
return Promise.resolve(jsonResponse({}));
|
||||
});
|
||||
|
||||
const { unmount } = render(<ResourcesView />);
|
||||
await waitFor(() => expect(rejectResources).toBeDefined());
|
||||
|
||||
unmount();
|
||||
rejectResources!(new Error('network down'));
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
|
||||
expect(toast.error).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('surfaces the server error on a failed prune instead of a false success (M-2)', async () => {
|
||||
mockedFetch.mockImplementation((url: string, opts?: RequestInit) => {
|
||||
if (url === '/system/prune/system' && opts?.method === 'POST') {
|
||||
return Promise.resolve(jsonResponse({ error: 'Prune blew up' }, { ok: false, status: 500 }));
|
||||
}
|
||||
if (url === '/system/resources') {
|
||||
return Promise.resolve(jsonResponse({ images: [], volumes: [], networks: [] }));
|
||||
}
|
||||
return Promise.resolve(jsonResponse({}));
|
||||
});
|
||||
|
||||
const user = userEvent.setup();
|
||||
render(<ResourcesView />);
|
||||
await waitFor(() => expect(mockedFetch).toHaveBeenCalledWith('/system/resources'));
|
||||
|
||||
await user.click(screen.getByRole('button', { name: /Prune Unused Images/ }));
|
||||
await user.click(await screen.findByRole('button', { name: /^Prune$/ }));
|
||||
|
||||
await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Prune blew up'));
|
||||
expect(toast.success).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user