fix folder causing 500 toast when deleted on another machine (#6551)

This commit is contained in:
Anthony Stirling
2026-06-09 18:00:02 +01:00
committed by GitHub
parent 1a0beaffc2
commit 502f6c1e4d
4 changed files with 350 additions and 27 deletions
@@ -24,6 +24,7 @@ import org.springframework.web.multipart.MaxUploadSizeExceededException;
import org.springframework.web.multipart.support.MissingServletRequestPartException;
import org.springframework.web.server.ResponseStatusException;
import org.springframework.web.servlet.NoHandlerFoundException;
import org.springframework.web.servlet.resource.NoResourceFoundException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
@@ -993,6 +994,49 @@ public class GlobalExceptionHandler {
.body(problemDetail);
}
/** Unmapped path → clean 404 instead of falling through to the generic 500 catch-all. */
@ExceptionHandler(NoResourceFoundException.class)
public ResponseEntity<ProblemDetail> handleNoResourceFound(
NoResourceFoundException ex, HttpServletRequest request) {
// /api/* miss = likely missing controller (operator-relevant); other paths = favicons,
// robots.txt, scanner noise. Demote the latter so prod logs aren't flooded.
String uri = request.getRequestURI();
if (uri != null && uri.startsWith("/api/")) {
log.warn("No resource at {}: {}", uri, ex.getMessage());
} else {
log.debug("No resource at {}: {}", uri, ex.getMessage());
}
String title = getLocalizedMessage("error.notFound.title", ErrorTitles.NOT_FOUND_DEFAULT);
String detail =
getLocalizedMessage(
"error.notFound.detail",
String.format(
"No endpoint found for %s %s",
request.getMethod(), request.getRequestURI()),
request.getMethod(),
request.getRequestURI());
ProblemDetail problemDetail =
createBaseProblemDetail(HttpStatus.NOT_FOUND, detail, request);
problemDetail.setType(URI.create(ErrorTypes.NOT_FOUND));
problemDetail.setTitle(title);
problemDetail.setProperty("title", title);
problemDetail.setProperty("method", request.getMethod());
addStandardHints(
problemDetail,
"error.notFound.hints",
List.of(
"Verify the URL path and HTTP method are correct.",
"Check the API base path and version if applicable.",
"Ensure there are no typos in the endpoint path."));
problemDetail.setProperty("actionRequired", "Use a valid endpoint URL and method.");
return ResponseEntity.status(HttpStatus.NOT_FOUND)
.contentType(PROBLEM_JSON)
.body(problemDetail);
}
/**
* Handle IllegalArgumentException.
*
@@ -18,6 +18,7 @@ import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.context.MessageSource;
import org.springframework.core.env.Environment;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.ProblemDetail;
import org.springframework.http.ResponseEntity;
@@ -27,6 +28,7 @@ import org.springframework.web.bind.MissingServletRequestParameterException;
import org.springframework.web.multipart.MaxUploadSizeExceededException;
import org.springframework.web.multipart.support.MissingServletRequestPartException;
import org.springframework.web.servlet.NoHandlerFoundException;
import org.springframework.web.servlet.resource.NoResourceFoundException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
@@ -208,6 +210,19 @@ class GlobalExceptionHandlerTest {
assertEquals(HttpStatus.NOT_FOUND, resp.getStatusCode());
}
// ---- NoResourceFoundException ----
// Regression guard: was falling through to the 500 catch-all.
@Test
void handleNoResourceFound_returns_404_not_500() {
when(request.getMethod()).thenReturn("GET");
NoResourceFoundException ex =
new NoResourceFoundException(HttpMethod.GET, "/api/v1/storage/folders", "");
ResponseEntity<ProblemDetail> resp = handler.handleNoResourceFound(ex, request);
assertEquals(HttpStatus.NOT_FOUND, resp.getStatusCode());
assertEquals("GET", resp.getBody().getProperties().get("method"));
}
// ---- IllegalArgumentException ----
@Test
@@ -3,6 +3,7 @@ import { describe, expect, test, vi, beforeEach } from "vitest";
import { render, screen, waitFor, act } from "@testing-library/react";
import { FolderProvider, useFolders } from "@app/contexts/FolderContext";
import { createFolderId, FolderId, FolderRecord } from "@app/types/folder";
import { expectConsole } from "@app/tests/failOnConsole";
/**
@@ -26,21 +27,43 @@ import { expectConsole } from "@app/tests/failOnConsole";
// require a running backend (folderSyncService) and a populated IDB
// (folderStorage). Both are out of scope for testing the error gate.
const mockList = vi.fn();
const mockUpdate = vi.fn();
const mockDelete = vi.fn();
vi.mock("@app/services/folderSyncService", () => ({
folderSyncService: {
list: () => mockList(),
create: vi.fn(),
update: vi.fn(),
delete: vi.fn(),
update: (...args: unknown[]) => mockUpdate(...args),
delete: (...args: unknown[]) => mockDelete(...args),
},
}));
// Stateful IDB mock - the revision-driven refresh re-reads getAllFolders
// after every state change, so a stateless [] mock would clobber pull results.
const { mockIdb } = vi.hoisted(() => ({
mockIdb: { folders: [] as { id: string }[] },
}));
vi.mock("@app/services/folderStorage", () => ({
folderStorage: {
getAllFolders: vi.fn().mockResolvedValue([]),
replaceAll: vi.fn().mockResolvedValue(undefined),
upsert: vi.fn().mockResolvedValue(undefined),
getAllFolders: vi.fn(() => Promise.resolve([...mockIdb.folders])),
replaceAll: vi.fn((next: { id: string }[]) => {
mockIdb.folders = [...next];
return Promise.resolve();
}),
upsert: vi.fn(),
upsertFolder: vi.fn((next: { id: string }) => {
mockIdb.folders = [
...mockIdb.folders.filter((f) => f.id !== next.id),
next,
];
return Promise.resolve();
}),
delete: vi.fn().mockResolvedValue(undefined),
removeFolders: vi.fn((ids: string[]) => {
const drop = new Set(ids);
mockIdb.folders = mockIdb.folders.filter((f) => !drop.has(f.id));
return Promise.resolve();
}),
},
}));
@@ -105,6 +128,8 @@ async function renderAndWaitForPull(): Promise<void> {
describe("FolderContext sync-banner gating", () => {
beforeEach(() => {
mockList.mockReset();
mockUpdate.mockReset();
mockDelete.mockReset();
});
test("401 (unauthorized) does NOT surface a banner", async () => {
@@ -157,3 +182,163 @@ describe("FolderContext sync-banner gating", () => {
expect(screen.getByTestId("reachable").textContent).toBe("false");
});
});
/** Per-folder mutation 404 = silent cleanup (drop subtree, no banner, pull). */
describe("FolderContext stale-folder 404 cleanup", () => {
beforeEach(() => {
mockList.mockReset();
mockUpdate.mockReset();
mockDelete.mockReset();
// Reset the stateful IDB mock so each test starts with an empty cache.
mockIdb.folders = [];
});
function makeFolder(name: string, parentFolderId: FolderId | null = null) {
return {
id: createFolderId(),
name,
parentFolderId,
createdAt: 0,
updatedAt: 0,
} as FolderRecord;
}
type ProbeApi = {
error: string | null;
folderCount: number;
currentFolderId: FolderId | null;
setCurrentFolderId: (id: FolderId | null) => void;
rename: (id: FolderId, name: string) => Promise<unknown>;
delete: (id: FolderId) => Promise<unknown>;
};
function ApiProbe(props: { onReady: (api: ProbeApi) => void }) {
const f = useFolders();
React.useEffect(() => {
props.onReady({
error: f.error,
folderCount: f.folders.length,
currentFolderId: f.currentFolderId,
setCurrentFolderId: f.setCurrentFolderId,
rename: (id, name) => f.renameFolder(id, name),
delete: (id) => f.deleteFolder(id),
});
}, [f, props]);
return (
<>
<div data-testid="error">{f.error ?? "<null>"}</div>
<div data-testid="reachable">{String(f.serverReachable)}</div>
<div data-testid="count">{f.folders.length}</div>
<div data-testid="current">{f.currentFolderId ?? "<null>"}</div>
</>
);
}
async function setupWithFolders(
initial: FolderRecord[],
): Promise<{ current: ProbeApi }> {
// Ref (not plain object) so tests see the latest f-bound api after re-renders.
mockList.mockResolvedValueOnce(initial);
const apiRef: { current: ProbeApi | null } = { current: null };
render(
<FolderProvider>
<ApiProbe onReady={(api) => (apiRef.current = api)} />
</FolderProvider>,
);
await waitFor(() =>
expect(screen.getByTestId("count").textContent).toBe(
String(initial.length),
),
);
if (!apiRef.current) throw new Error("ApiProbe never reported ready");
return apiRef as { current: ProbeApi };
}
test("renameFolder 404 silently drops folder + descendants, no banner", async () => {
const parent = makeFolder("parent");
const child = makeFolder("child", parent.id);
const sibling = makeFolder("sibling");
const api = await setupWithFolders([parent, child, sibling]);
// Convergence pull returns just sibling after the local drop.
mockList.mockResolvedValueOnce([sibling]);
mockUpdate.mockRejectedValueOnce(axiosError(404, "Folder not found"));
await act(async () => {
const result = await api.current.rename(parent.id, "newname");
expect(result).toBeNull();
});
expect(screen.getByTestId("error").textContent).toBe("<null>");
await waitFor(() =>
expect(screen.getByTestId("count").textContent).toBe("1"),
);
expect(screen.getByTestId("reachable").textContent).toBe("true");
});
test("deleteFolder 404 is treated as already-deleted, returns [id], no banner", async () => {
const target = makeFolder("target");
const other = makeFolder("other");
const api = await setupWithFolders([target, other]);
mockList.mockResolvedValueOnce([other]);
mockDelete.mockRejectedValueOnce(axiosError(404, "Folder not found"));
let result: unknown;
await act(async () => {
result = await api.current.delete(target.id);
});
expect(result).toEqual([target.id]);
expect(screen.getByTestId("error").textContent).toBe("<null>");
await waitFor(() =>
expect(screen.getByTestId("count").textContent).toBe("1"),
);
expect(screen.getByTestId("reachable").textContent).toBe("true");
});
test("stale 404 strand-resets currentFolderId when user is inside the dropped subtree", async () => {
// User is parked inside the about-to-be-deleted subtree → must navigate out.
const parent = makeFolder("parent");
const child = makeFolder("child", parent.id);
const sibling = makeFolder("sibling");
const api = await setupWithFolders([parent, child, sibling]);
act(() => {
api.current.setCurrentFolderId(child.id);
});
await waitFor(() =>
expect(screen.getByTestId("current").textContent).toBe(child.id),
);
mockList.mockResolvedValueOnce([sibling]);
mockUpdate.mockRejectedValueOnce(axiosError(404, "Folder not found"));
await act(async () => {
await api.current.rename(parent.id, "doomed-rename");
});
// ROOT_FOLDER_ID is null → probe renders "<null>".
expect(screen.getByTestId("current").textContent).toBe("<null>");
expect(screen.getByTestId("error").textContent).toBe("<null>");
});
test("non-404 mutation errors still surface (regression guard)", async () => {
const target = makeFolder("target");
const api = await setupWithFolders([target]);
mockUpdate.mockRejectedValueOnce(axiosError(500, "boom"));
let threw = false;
await act(async () => {
try {
await api.current.rename(target.id, "newname");
} catch {
threw = true;
}
});
expect(threw).toBe(true);
expect(screen.getByTestId("count").textContent).toBe("1");
expect(screen.getByTestId("error").textContent).not.toBe("<null>");
});
});
@@ -178,6 +178,40 @@ function reachabilityFromError(err: unknown): boolean {
return status !== undefined && status >= 400 && status < 500;
}
/** Root + every local descendant via parentFolderId. Bounded for corrupted chains. */
function collectLocalSubtreeIds(
rootId: FolderId,
folders: FolderRecord[],
): Set<FolderId> {
const childrenByParent = new Map<FolderId, FolderId[]>();
for (const f of folders) {
if (f.parentFolderId === null) continue;
const list = childrenByParent.get(f.parentFolderId) ?? [];
list.push(f.id);
childrenByParent.set(f.parentFolderId, list);
}
const result = new Set<FolderId>([rootId]);
const stack: FolderId[] = [rootId];
const MAX_LOCAL_SUBTREE_NODES = 10_000;
while (stack.length > 0 && result.size < MAX_LOCAL_SUBTREE_NODES) {
const cur = stack.pop()!;
const children = childrenByParent.get(cur);
if (!children) continue;
for (const childId of children) {
if (!result.has(childId)) {
result.add(childId);
stack.push(childId);
}
}
}
return result;
}
/** Extract HTTP status off an axios-style error, or undefined on network failure. */
function errorStatus(err: unknown): number | undefined {
return (err as { response?: { status?: number } })?.response?.status;
}
/**
* True if `currentId` or any of its ancestors is in `removedSet`. Walks up via
* `parentFolderId` using the pre-removal `folders` snapshot so the chain is
@@ -390,24 +424,58 @@ export function FolderProvider({ children }: FolderProviderProps) {
// ─── mutations: server-first, cache update on success ──────────────
// Lifted up so handleStaleFolder below can reuse the file-detach primitive.
const { clearFolderForFiles } = useIndexedDB();
/** Treat a per-folder 404 as "deleted elsewhere": drop subtree, strand-reset, pull. */
const handleStaleFolder = useCallback(
(staleId: FolderId, foldersSnapshot: FolderRecord[]) => {
const subtree = collectLocalSubtreeIds(staleId, foldersSnapshot);
if (mountedRef.current) {
setServerReachable(true);
setError(null);
setFolders((prev) => prev.filter((f) => !subtree.has(f.id)));
if (
currentFolderId !== null &&
shouldStrandedReset(currentFolderId, subtree, foldersSnapshot)
) {
setCurrentFolderId(ROOT_FOLDER_ID);
}
}
const subtreeIds = [...subtree];
// Best-effort - pullFromServer is authoritative if these miss.
void folderStorage
.removeFolders(subtreeIds)
.catch((e) => console.warn("[FolderContext] stale cache cleanup", e));
void clearFolderForFiles(subtreeIds).catch((e) =>
console.warn("[FolderContext] stale file-folder cleanup", e),
);
bumpFolderRevision();
void pullFromServer();
},
[bumpFolderRevision, clearFolderForFiles, currentFolderId, pullFromServer],
);
/**
* Centralised mutation wrapper:
* 1. Calls the server op.
* 2. On success: flips `serverReachable=true`, updates in-memory state
* from the server response, then best-effort writes the cache (cache
* failure does NOT roll back; the in-memory truth came from the server).
* 3. On failure: updates `serverReachable` per the error class, surfaces
* via {@link setError}, re-throws so the caller's dialog can stay open.
* Server-first mutation wrapper. On 404 with `staleFolderId`, hands off to
* handleStaleFolder and resolves `null`; other errors surface + re-throw.
*/
const runFolderMutation = useCallback(
async <T,>(
serverOp: () => Promise<T>,
onSuccess: (result: T) => void | Promise<void>,
): Promise<T> => {
staleFolderId: FolderId | null = null,
): Promise<T | null> => {
let result: T;
try {
result = await serverOp();
} catch (err) {
if (errorStatus(err) === 404 && staleFolderId !== null) {
// `folders` is a closure snapshot; functional setFolders + the pull
// keep this race-safe even if a concurrent mutation shifted state.
handleStaleFolder(staleFolderId, folders);
return null;
}
if (mountedRef.current) {
setServerReachable(reachabilityFromError(err));
setError(formatServerError(err));
@@ -421,18 +489,13 @@ export function FolderProvider({ children }: FolderProviderProps) {
try {
await onSuccess(result);
} catch (cacheErr) {
// The server is authoritative - a cache write failure must not be
// surfaced as if the operation failed. Log + leave the in-memory
// state authoritative; next pullFromServer will re-seed the cache.
console.warn(
"[FolderContext] cache update failed after successful mutation",
cacheErr,
);
// Cache write failure is non-fatal; next pull re-seeds.
console.warn("[FolderContext] cache update after mutation", cacheErr);
}
bumpFolderRevision();
return result;
},
[bumpFolderRevision],
[bumpFolderRevision, folders, handleStaleFolder],
);
const createFolder = useCallback(
@@ -441,10 +504,9 @@ export function FolderProvider({ children }: FolderProviderProps) {
parentFolderId: FolderId | null = currentFolderId,
): Promise<FolderRecord> => {
const color = pickFolderColor(name);
// Generate id client-side so the server's idempotency check makes
// retries safe (network blip → second POST returns the same row).
// Client-side id makes server idempotency check safe on retry.
const id = createFolderId();
return runFolderMutation(
const result = await runFolderMutation(
() =>
folderSyncService.create({
id,
@@ -460,6 +522,11 @@ export function FolderProvider({ children }: FolderProviderProps) {
await folderStorage.upsertFolder(record);
},
);
// No staleFolderId passed → null branch can't fire; defensive throw.
if (result === null) {
throw new Error("createFolder unexpectedly returned null");
}
return result;
},
[currentFolderId, runFolderMutation],
);
@@ -474,6 +541,7 @@ export function FolderProvider({ children }: FolderProviderProps) {
);
await folderStorage.upsertFolder(record);
},
id,
);
},
[runFolderMutation],
@@ -493,6 +561,7 @@ export function FolderProvider({ children }: FolderProviderProps) {
);
await folderStorage.upsertFolder(record);
},
id,
);
},
[runFolderMutation],
@@ -515,13 +584,12 @@ export function FolderProvider({ children }: FolderProviderProps) {
);
await folderStorage.upsertFolder(record);
},
id,
);
},
[runFolderMutation],
);
const { clearFolderForFiles } = useIndexedDB();
const deleteFolder = useCallback(
async (id: FolderId): Promise<FolderId[]> => {
// Custom path (not runFolderMutation) because we have two best-effort
@@ -532,6 +600,11 @@ export function FolderProvider({ children }: FolderProviderProps) {
try {
removed = await folderSyncService.delete(id);
} catch (err) {
if (errorStatus(err) === 404) {
// Already gone; treat as success. Return [id]; pull is authoritative.
handleStaleFolder(id, folders);
return [id];
}
if (mountedRef.current) {
setServerReachable(reachabilityFromError(err));
setError(formatServerError(err));
@@ -580,7 +653,13 @@ export function FolderProvider({ children }: FolderProviderProps) {
}
return removed;
},
[bumpFolderRevision, clearFolderForFiles, currentFolderId, folders],
[
bumpFolderRevision,
clearFolderForFiles,
currentFolderId,
folders,
handleStaleFolder,
],
);
const value = useMemo<FolderContextValue>(