perf(site-finder): honour AbortSignal в poll-цикле useParcelAnalyzeQuery (#1248)
queryFn игнорировал TanStack v5 AbortSignal: first POST /analyze, poll GET /fetch-status и re-POST не передавали signal, цикл спал на non-abortable setTimeout без проверки signal.aborted. При unmount / смене cad|horizon цикл жил до 2-мин кэпа, дёргая GET /fetch-status каждые 2с + POST /analyze на каждый ready — зомби-поллинг. Проброс signal во все 3 fetch, sleep через shared abortableSleep (extracted из useSiteAnalysis #1242), throw AbortError перед итерацией. +3 vitest (abort + happy-path). Closes #1248
This commit is contained in:
parent
3cf9fad683
commit
16e505326d
4 changed files with 297 additions and 30 deletions
|
|
@ -4,6 +4,7 @@ import { useMutation } from "@tanstack/react-query";
|
||||||
import { useCallback, useRef, useState } from "react";
|
import { useCallback, useRef, useState } from "react";
|
||||||
|
|
||||||
import { HTTPError, apiFetch, apiFetchWithStatus } from "@/lib/api";
|
import { HTTPError, apiFetch, apiFetchWithStatus } from "@/lib/api";
|
||||||
|
import { abortableSleep } from "@/lib/abortableSleep";
|
||||||
import type { ParcelAnalysis } from "@/types/site-finder";
|
import type { ParcelAnalysis } from "@/types/site-finder";
|
||||||
|
|
||||||
// #93 — on-demand cadastre fetch flow.
|
// #93 — on-demand cadastre fetch flow.
|
||||||
|
|
@ -51,31 +52,6 @@ export interface AnalyzeOptions {
|
||||||
weights?: Record<string, number> | null;
|
weights?: Record<string, number> | null;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Sleep, but resolve early if the AbortSignal fires.
|
|
||||||
*
|
|
||||||
* Используем чтобы цикл polling'а пробуждался сразу при cancel(),
|
|
||||||
* а не ждал полные POLL_INTERVAL_MS до следующей проверки.
|
|
||||||
*/
|
|
||||||
function abortableSleep(ms: number, signal: AbortSignal): Promise<void> {
|
|
||||||
return new Promise((resolve) => {
|
|
||||||
if (signal.aborted) {
|
|
||||||
resolve();
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
const timer = setTimeout(() => {
|
|
||||||
signal.removeEventListener("abort", onAbort);
|
|
||||||
resolve();
|
|
||||||
}, ms);
|
|
||||||
const onAbort = () => {
|
|
||||||
clearTimeout(timer);
|
|
||||||
signal.removeEventListener("abort", onAbort);
|
|
||||||
resolve();
|
|
||||||
};
|
|
||||||
signal.addEventListener("abort", onAbort, { once: true });
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Custom hook для analyze flow с graceful on-demand fetch fallback.
|
* Custom hook для analyze flow с graceful on-demand fetch fallback.
|
||||||
*
|
*
|
||||||
|
|
|
||||||
247
frontend/src/lib/__tests__/useParcelAnalyzeQuery.test.ts
Normal file
247
frontend/src/lib/__tests__/useParcelAnalyzeQuery.test.ts
Normal file
|
|
@ -0,0 +1,247 @@
|
||||||
|
/**
|
||||||
|
* Regression coverage for issue #1248 (P3 performance).
|
||||||
|
*
|
||||||
|
* Bug: `useParcelAnalyzeQuery`'s queryFn polled GET /fetch-status (+ re-POST
|
||||||
|
* /analyze) in a `for (i < 60)` loop with a non-abortable `setTimeout` and never
|
||||||
|
* read the TanStack Query v5 AbortSignal. On unmount / смена cad|horizon the
|
||||||
|
* loop kept firing requests up to the 2-min hard cap — zombie-polling.
|
||||||
|
*
|
||||||
|
* Fix: queryFn destructures `{ signal }`, forwards it to every apiFetch call,
|
||||||
|
* sleeps via `abortableSleep`, and throws AbortError before each iteration once
|
||||||
|
* `signal.aborted`. The sibling mutation flow (useSiteAnalysis #1242) already
|
||||||
|
* did this; both now share `@/lib/abortableSleep`.
|
||||||
|
*
|
||||||
|
* Strategy: `@tanstack/react-query` is mocked so the hook's `useQuery(options)`
|
||||||
|
* call simply captures the options object. We then invoke `options.queryFn`
|
||||||
|
* directly with a real AbortSignal and a per-URL `fetch` stub, under fake
|
||||||
|
* timers, and assert on abort behaviour + the happy path.
|
||||||
|
*/
|
||||||
|
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||||
|
|
||||||
|
// ── Capture the options passed to useQuery ───────────────────────────────────
|
||||||
|
// The captured options carry the polling queryFn we want to drive directly.
|
||||||
|
interface CapturedQueryOptions {
|
||||||
|
queryKey: readonly unknown[];
|
||||||
|
queryFn: (ctx: { signal: AbortSignal }) => Promise<unknown>;
|
||||||
|
}
|
||||||
|
|
||||||
|
// vi.mock is hoisted; keep the captured ref in a hoisted holder so both the
|
||||||
|
// factory and the tests can reach it.
|
||||||
|
const captured = vi.hoisted<{ options: CapturedQueryOptions | null }>(() => ({
|
||||||
|
options: null,
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@tanstack/react-query", () => ({
|
||||||
|
keepPreviousData: Symbol("keepPreviousData"),
|
||||||
|
useQuery: (options: CapturedQueryOptions) => {
|
||||||
|
captured.options = options;
|
||||||
|
return {};
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { useParcelAnalyzeQuery } from "../site-finder-api";
|
||||||
|
|
||||||
|
// ── fetch router ─────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
interface MockSpec {
|
||||||
|
status: number;
|
||||||
|
body?: unknown;
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Minimal Response stub — only the fields apiFetch/apiFetchWithStatus read. */
|
||||||
|
function makeResponse({ status, body }: MockSpec): Response {
|
||||||
|
const text = body == null ? "" : JSON.stringify(body);
|
||||||
|
return {
|
||||||
|
ok: status >= 200 && status < 300,
|
||||||
|
status,
|
||||||
|
text: () => Promise.resolve(text),
|
||||||
|
} as unknown as Response;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Routes the stubbed fetch by URL + method and records every call so we can
|
||||||
|
* assert that nothing fires after abort. `statusSeq` is the queue of
|
||||||
|
* /fetch-status responses, consumed in order.
|
||||||
|
*/
|
||||||
|
function installFetchRouter(opts: {
|
||||||
|
firstAnalyze: MockSpec;
|
||||||
|
statusSeq: MockSpec[];
|
||||||
|
secondAnalyze?: MockSpec;
|
||||||
|
}) {
|
||||||
|
const calls: Array<{ url: string; method: string }> = [];
|
||||||
|
let analyzeCount = 0;
|
||||||
|
let statusIdx = 0;
|
||||||
|
|
||||||
|
const fn = vi.fn((input: RequestInfo | URL, init?: RequestInit) => {
|
||||||
|
const url = String(input);
|
||||||
|
const method = (init?.method ?? "GET").toUpperCase();
|
||||||
|
calls.push({ url, method });
|
||||||
|
|
||||||
|
// Mirror the browser: a pre-aborted signal makes fetch reject immediately.
|
||||||
|
if (init?.signal?.aborted) {
|
||||||
|
return Promise.reject(new DOMException("Aborted", "AbortError"));
|
||||||
|
}
|
||||||
|
|
||||||
|
if (url.includes("/analyze")) {
|
||||||
|
analyzeCount += 1;
|
||||||
|
const spec =
|
||||||
|
analyzeCount === 1
|
||||||
|
? opts.firstAnalyze
|
||||||
|
: (opts.secondAnalyze ?? { status: 200, body: {} });
|
||||||
|
return Promise.resolve(makeResponse(spec));
|
||||||
|
}
|
||||||
|
if (url.includes("/fetch-status")) {
|
||||||
|
const spec = opts.statusSeq[statusIdx] ?? {
|
||||||
|
status: 200,
|
||||||
|
body: { status: "fetching" },
|
||||||
|
};
|
||||||
|
statusIdx += 1;
|
||||||
|
return Promise.resolve(makeResponse(spec));
|
||||||
|
}
|
||||||
|
return Promise.reject(new Error(`unexpected fetch: ${method} ${url}`));
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.stubGlobal("fetch", fn);
|
||||||
|
return {
|
||||||
|
calls,
|
||||||
|
statusCalls: () =>
|
||||||
|
calls.filter((c) => c.url.includes("/fetch-status")).length,
|
||||||
|
analyzeCalls: () => calls.filter((c) => c.url.includes("/analyze")).length,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const CAD = "66:41:0701045:42";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Run the hook so the mocked `useQuery` captures its options, then return the
|
||||||
|
* polling queryFn. Reads `captured.options` via a fresh binding so TS control-
|
||||||
|
* flow doesn't pin it (the hook mutates it opaquely through the mock).
|
||||||
|
*
|
||||||
|
* `useQuery` is fully mocked (it just records its options, no React state), so
|
||||||
|
* the rules-of-hooks invariant does not apply to this call — disable locally.
|
||||||
|
*/
|
||||||
|
function getQueryFn(): CapturedQueryOptions["queryFn"] {
|
||||||
|
// eslint-disable-next-line react-hooks/rules-of-hooks
|
||||||
|
useParcelAnalyzeQuery(CAD, 12);
|
||||||
|
const options = captured.options;
|
||||||
|
if (options === null) throw new Error("useQuery options not captured");
|
||||||
|
return options.queryFn;
|
||||||
|
}
|
||||||
|
|
||||||
|
function runQueryFn(signal: AbortSignal): Promise<unknown> {
|
||||||
|
return getQueryFn()({ signal });
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("useParcelAnalyzeQuery — poll loop honours AbortSignal (#1248)", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
captured.options = null;
|
||||||
|
vi.useFakeTimers();
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
vi.useRealTimers();
|
||||||
|
vi.unstubAllGlobals();
|
||||||
|
vi.restoreAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("aborts the poll loop and stops issuing requests after abort", async () => {
|
||||||
|
const controller = new AbortController();
|
||||||
|
const router = installFetchRouter({
|
||||||
|
firstAnalyze: {
|
||||||
|
status: 202,
|
||||||
|
body: { status: "fetching", job_id: 1, eta_seconds: 30 },
|
||||||
|
},
|
||||||
|
// Always "fetching" → the loop would otherwise run to the 60-iter cap.
|
||||||
|
statusSeq: Array.from({ length: 60 }, () => ({
|
||||||
|
status: 200,
|
||||||
|
body: {
|
||||||
|
status: "fetching",
|
||||||
|
job_id: 1,
|
||||||
|
error_msg: null,
|
||||||
|
eta_seconds: 10,
|
||||||
|
},
|
||||||
|
})),
|
||||||
|
});
|
||||||
|
|
||||||
|
const promise = runQueryFn(controller.signal).catch((e: unknown) => e);
|
||||||
|
|
||||||
|
// First POST /analyze (202) resolves and the loop enters its first sleep.
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
expect(router.analyzeCalls()).toBe(1);
|
||||||
|
|
||||||
|
// One poll cycle: sleep 2s → GET /fetch-status ("fetching").
|
||||||
|
await vi.advanceTimersByTimeAsync(2000);
|
||||||
|
expect(router.statusCalls()).toBeGreaterThanOrEqual(1);
|
||||||
|
|
||||||
|
// Abort mid-flight (как unmount / смена cad). abortableSleep wakes at once.
|
||||||
|
controller.abort();
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
|
||||||
|
const result = await promise;
|
||||||
|
expect(result).toBeInstanceOf(DOMException);
|
||||||
|
expect((result as DOMException).name).toBe("AbortError");
|
||||||
|
|
||||||
|
// No further requests after abort — even if the full 2-min cap elapses.
|
||||||
|
const callsAtAbort = router.calls.length;
|
||||||
|
await vi.advanceTimersByTimeAsync(60 * 2000);
|
||||||
|
expect(router.calls.length).toBe(callsAtAbort);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not poll at all when the signal is already aborted", async () => {
|
||||||
|
const controller = new AbortController();
|
||||||
|
controller.abort();
|
||||||
|
const router = installFetchRouter({
|
||||||
|
firstAnalyze: {
|
||||||
|
status: 202,
|
||||||
|
body: { status: "fetching", job_id: 1, eta_seconds: 30 },
|
||||||
|
},
|
||||||
|
statusSeq: [],
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await runQueryFn(controller.signal).catch((e: unknown) => e);
|
||||||
|
|
||||||
|
expect(result).toBeInstanceOf(DOMException);
|
||||||
|
expect((result as DOMException).name).toBe("AbortError");
|
||||||
|
// First POST /analyze sees the aborted signal → rejects, never polls.
|
||||||
|
expect(router.statusCalls()).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("happy path: polls until ready then resolves with the 200 analysis", async () => {
|
||||||
|
const controller = new AbortController();
|
||||||
|
const payload = { cad_num: CAD, score: 72, source: "nspd" };
|
||||||
|
const router = installFetchRouter({
|
||||||
|
firstAnalyze: {
|
||||||
|
status: 202,
|
||||||
|
body: { status: "fetching", job_id: 1, eta_seconds: 30 },
|
||||||
|
},
|
||||||
|
statusSeq: [
|
||||||
|
{
|
||||||
|
status: 200,
|
||||||
|
body: {
|
||||||
|
status: "fetching",
|
||||||
|
job_id: 1,
|
||||||
|
error_msg: null,
|
||||||
|
eta_seconds: 10,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
status: 200,
|
||||||
|
body: { status: "ready", job_id: 1, error_msg: null, eta_seconds: 0 },
|
||||||
|
},
|
||||||
|
],
|
||||||
|
secondAnalyze: { status: 200, body: payload },
|
||||||
|
});
|
||||||
|
|
||||||
|
const promise = runQueryFn(controller.signal);
|
||||||
|
|
||||||
|
// first POST 202 → sleep → status(fetching) → sleep → status(ready) → re-POST 200
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
await vi.advanceTimersByTimeAsync(2000);
|
||||||
|
await vi.advanceTimersByTimeAsync(2000);
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
|
||||||
|
await expect(promise).resolves.toEqual(payload);
|
||||||
|
expect(router.analyzeCalls()).toBe(2); // first + re-trigger
|
||||||
|
expect(router.statusCalls()).toBe(2);
|
||||||
|
});
|
||||||
|
});
|
||||||
32
frontend/src/lib/abortableSleep.ts
Normal file
32
frontend/src/lib/abortableSleep.ts
Normal file
|
|
@ -0,0 +1,32 @@
|
||||||
|
/**
|
||||||
|
* Sleep for `ms`, but resolve early if the AbortSignal fires.
|
||||||
|
*
|
||||||
|
* Используем в poll-циклах (analyze / fetch-status) чтобы цикл пробуждался
|
||||||
|
* сразу при abort (unmount, смена cad/horizon, cancel), а не ждал полные `ms`
|
||||||
|
* до следующей проверки `signal.aborted`. Без этого осиротевший цикл живёт до
|
||||||
|
* hard-cap (60 × 2s = 2 мин), сыпя GET /fetch-status + POST /analyze — зомби-
|
||||||
|
* поллинг (#1248, sibling #1242).
|
||||||
|
*
|
||||||
|
* Резолвится (не реджектится) — вызывающий сам решает бросать ли AbortError,
|
||||||
|
* проверив `signal.aborted` после await.
|
||||||
|
*/
|
||||||
|
export function abortableSleep(
|
||||||
|
ms: number,
|
||||||
|
signal?: AbortSignal,
|
||||||
|
): Promise<void> {
|
||||||
|
return new Promise((resolve) => {
|
||||||
|
if (signal?.aborted) {
|
||||||
|
resolve();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
const onAbort = () => {
|
||||||
|
clearTimeout(timer);
|
||||||
|
resolve();
|
||||||
|
};
|
||||||
|
const timer = setTimeout(() => {
|
||||||
|
signal?.removeEventListener("abort", onAbort);
|
||||||
|
resolve();
|
||||||
|
}, ms);
|
||||||
|
signal?.addEventListener("abort", onAbort, { once: true });
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
@ -10,6 +10,7 @@
|
||||||
|
|
||||||
import { keepPreviousData, useQuery } from "@tanstack/react-query";
|
import { keepPreviousData, useQuery } from "@tanstack/react-query";
|
||||||
import { HTTPError, apiFetch, apiFetchWithStatus } from "@/lib/api";
|
import { HTTPError, apiFetch, apiFetchWithStatus } from "@/lib/api";
|
||||||
|
import { abortableSleep } from "@/lib/abortableSleep";
|
||||||
import type {
|
import type {
|
||||||
AnalyzeAcceptedResponse,
|
AnalyzeAcceptedResponse,
|
||||||
FetchStatusResponse,
|
FetchStatusResponse,
|
||||||
|
|
@ -414,13 +415,18 @@ const ANALYZE_POLL_MAX_ITERATIONS = 60; // 60 × 2s = 2 min hard cap
|
||||||
export function useParcelAnalyzeQuery(cad: string, horizon: number = 12) {
|
export function useParcelAnalyzeQuery(cad: string, horizon: number = 12) {
|
||||||
return useQuery({
|
return useQuery({
|
||||||
queryKey: ["parcel-analyze", cad, horizon],
|
queryKey: ["parcel-analyze", cad, horizon],
|
||||||
queryFn: async (): Promise<ParcelAnalyzeResponse> => {
|
// TanStack Query v5 passes an AbortSignal in the queryFn context; it aborts
|
||||||
|
// on unmount and whenever the queryKey changes (смена cad/horizon). Thread
|
||||||
|
// it through the POST/GET fetches and check it before each poll iteration so
|
||||||
|
// the loop dies immediately instead of zombie-polling /fetch-status + re-
|
||||||
|
// POSTing /analyze up to the 2-min hard cap (#1248).
|
||||||
|
queryFn: async ({ signal }): Promise<ParcelAnalyzeResponse> => {
|
||||||
if (MOCK_ANALYZE) {
|
if (MOCK_ANALYZE) {
|
||||||
// Return fixture data regardless of cad/horizon — for dev only
|
// Return fixture data regardless of cad/horizon — for dev only
|
||||||
return fixtureAnalyze as ParcelAnalyzeResponse;
|
return fixtureAnalyze as ParcelAnalyzeResponse;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Backend accepts ?horizon= ∈ {6,12,18,24} (default 12; 422 otherwise, #995).
|
// Backend accepts ?horizon= ∈ {6,12,18} (default 12; 422 otherwise, #995).
|
||||||
const analyzeUrl = `/api/v1/parcels/${encodeURIComponent(
|
const analyzeUrl = `/api/v1/parcels/${encodeURIComponent(
|
||||||
cad,
|
cad,
|
||||||
)}/analyze?horizon=${horizon}`;
|
)}/analyze?horizon=${horizon}`;
|
||||||
|
|
@ -429,7 +435,7 @@ export function useParcelAnalyzeQuery(cad: string, horizon: number = 12) {
|
||||||
// Accepted code instead of treating it as a successful payload.
|
// Accepted code instead of treating it as a successful payload.
|
||||||
const first = await apiFetchWithStatus<
|
const first = await apiFetchWithStatus<
|
||||||
ParcelAnalyzeResponse | AnalyzeAcceptedResponse
|
ParcelAnalyzeResponse | AnalyzeAcceptedResponse
|
||||||
>(analyzeUrl, { method: "POST" });
|
>(analyzeUrl, { method: "POST", signal });
|
||||||
|
|
||||||
// 200 → geometry was cached, full analysis is ready.
|
// 200 → geometry was cached, full analysis is ready.
|
||||||
if (first.status === 200) {
|
if (first.status === 200) {
|
||||||
|
|
@ -441,16 +447,22 @@ export function useParcelAnalyzeQuery(cad: string, horizon: number = 12) {
|
||||||
// pending throughout, so the page's "Анализируем участок…" screen shows.
|
// pending throughout, so the page's "Анализируем участок…" screen shows.
|
||||||
if (first.status === 202) {
|
if (first.status === 202) {
|
||||||
for (let i = 0; i < ANALYZE_POLL_MAX_ITERATIONS; i++) {
|
for (let i = 0; i < ANALYZE_POLL_MAX_ITERATIONS; i++) {
|
||||||
await new Promise((r) => setTimeout(r, ANALYZE_POLL_INTERVAL_MS));
|
// abortableSleep wakes immediately on abort; then bail before issuing
|
||||||
|
// any further requests (signal also aborts the in-flight fetches).
|
||||||
|
await abortableSleep(ANALYZE_POLL_INTERVAL_MS, signal);
|
||||||
|
if (signal.aborted) {
|
||||||
|
throw new DOMException("Анализ отменён", "AbortError");
|
||||||
|
}
|
||||||
const status = await apiFetch<FetchStatusResponse>(
|
const status = await apiFetch<FetchStatusResponse>(
|
||||||
`/api/v1/parcels/${encodeURIComponent(cad)}/fetch-status`,
|
`/api/v1/parcels/${encodeURIComponent(cad)}/fetch-status`,
|
||||||
|
{ signal },
|
||||||
);
|
);
|
||||||
if (status.status === "ready") {
|
if (status.status === "ready") {
|
||||||
// Re-POST should now be 200. If it races back to 202, keep polling
|
// Re-POST should now be 200. If it races back to 202, keep polling
|
||||||
// rather than returning the stub (symmetry with the first request).
|
// rather than returning the stub (symmetry with the first request).
|
||||||
const second = await apiFetchWithStatus<
|
const second = await apiFetchWithStatus<
|
||||||
ParcelAnalyzeResponse | AnalyzeAcceptedResponse
|
ParcelAnalyzeResponse | AnalyzeAcceptedResponse
|
||||||
>(analyzeUrl, { method: "POST" });
|
>(analyzeUrl, { method: "POST", signal });
|
||||||
if (second.status === 200) {
|
if (second.status === 200) {
|
||||||
return second.body as ParcelAnalyzeResponse;
|
return second.body as ParcelAnalyzeResponse;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue