Skip to content

Commit 494edcc

Browse files
authored
perf(connectors): singleflight ensureWarehouseRunning per warehouse (#420)
* perf(connectors): singleflight ensureWarehouseRunning per warehouse Concurrent analytics queries on a cold warehouse now share one poll loop and a single warehouses.start call. Joiners receive broadcast onStatus updates with last-status replay; ref-counted abort cancels only when all waiters leave. Closes #419 Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> * refactor(appkit): simplify warehouse readiness singleflight helpers Consolidate subscribe/join/spawn into focused helpers and tighten the abort-race handler without changing behavior. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> * fix(connectors): grace-period abort for warehouse readiness singleflight Delay shared poll cancellation by 100ms so React StrictMode unmount→remount can rejoin the in-flight warmup instead of surfacing UPSTREAM_ERROR on first render. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> * fix(appkit): run-to-completion readiness and StrictMode abort handling Let shared warehouse warmup finish when waiters disconnect instead of a brittle grace timer. Classify cancellations as STREAM_ABORTED, suppress error SSE frames for them, and ignore late envelopes on aborted hooks. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> * fix(connectors): hybrid orphan abort for warehouse readiness Abort shared readiness polls on the next microtask when all waiters leave before warehouses.start, so true orphans do not poll until timeout. After start is issued, the poll runs to completion so the warm-path cache stays primed. Synchronous StrictMode remounts rejoin before the microtask. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> --------- Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> Co-authored-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
1 parent 79c5055 commit 494edcc

7 files changed

Lines changed: 607 additions & 126 deletions

File tree

packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts

Lines changed: 110 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,37 @@
11
import { act, renderHook, waitFor } from "@testing-library/react";
22
import { afterEach, describe, expect, test, vi } from "vitest";
33

4+
let capturedCallbacks: {
5+
onMessage?: (msg: { data: string }) => void;
6+
onError?: (err: Error) => void;
7+
signal?: AbortSignal;
8+
} = {};
9+
10+
const mockFetchArrow = vi.fn();
11+
const mockProcessArrowBuffer = vi.fn();
12+
413
// Mock connectSSE so the hook does not attempt a real network request.
5-
const mockConnectSSE = vi.fn().mockImplementation((_opts: unknown) => {
6-
// Return a never-resolving promise; tests don't need the result.
7-
return new Promise<void>(() => {});
8-
});
14+
const mockConnectSSE = vi
15+
.fn()
16+
.mockImplementation(
17+
(opts: {
18+
onMessage?: (msg: { data: string }) => void;
19+
onError?: (err: Error) => void;
20+
signal?: AbortSignal;
21+
}) => {
22+
capturedCallbacks = {
23+
onMessage: opts.onMessage,
24+
onError: opts.onError,
25+
signal: opts.signal,
26+
};
27+
return new Promise<void>(() => {});
28+
},
29+
);
930

1031
vi.mock("@/js", () => ({
1132
ArrowClient: {
12-
fetchArrow: vi.fn(),
13-
processArrowBuffer: vi.fn(),
33+
fetchArrow: (...args: unknown[]) => mockFetchArrow(...args),
34+
processArrowBuffer: (...args: unknown[]) => mockProcessArrowBuffer(...args),
1435
},
1536
connectSSE: (...args: unknown[]) => mockConnectSSE(...args),
1637
}));
@@ -22,29 +43,32 @@ vi.mock("../use-query-hmr", () => ({
2243

2344
import { useAnalyticsQuery } from "../use-analytics-query";
2445

46+
function markAborted() {
47+
const sig = capturedCallbacks.signal;
48+
if (!sig) throw new Error("signal not captured yet");
49+
Object.defineProperty(sig, "aborted", { value: true, configurable: true });
50+
}
51+
2552
describe("useAnalyticsQuery", () => {
2653
afterEach(() => {
54+
capturedCallbacks = {};
2755
vi.clearAllMocks();
2856
});
2957

3058
test("does not refetch when params object is structurally equal across renders", () => {
31-
// Each render passes a fresh object literal — the common footgun.
3259
const { rerender } = renderHook(
3360
({ limit }: { limit: number }) =>
3461
// biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests
3562
useAnalyticsQuery("test_query" as any, { limit } as any),
3663
{ initialProps: { limit: 10 } },
3764
);
3865

39-
// Initial render triggers exactly one connection.
4066
expect(mockConnectSSE).toHaveBeenCalledTimes(1);
4167

42-
// Re-render with structurally-equal-but-new-reference params.
4368
rerender({ limit: 10 });
4469
rerender({ limit: 10 });
4570
rerender({ limit: 10 });
4671

47-
// Should NOT have refetched — the hook stabilized the params reference.
4872
expect(mockConnectSSE).toHaveBeenCalledTimes(1);
4973
});
5074

@@ -93,27 +117,26 @@ describe("useAnalyticsQuery", () => {
93117

94118
describe("warehouse_status", () => {
95119
test("surfaces warehouseStatus while waiting and clears loading on result", async () => {
96-
// Capture the connectSSE options so we can drive onMessage manually.
97120
let capturedOnMessage:
98121
| ((msg: { id: string; data: string }) => void)
99122
| null = null;
100-
mockConnectSSE.mockImplementationOnce((opts: any) => {
101-
capturedOnMessage = opts.onMessage;
102-
return new Promise<void>(() => {});
103-
});
123+
mockConnectSSE.mockImplementationOnce(
124+
(opts: { onMessage?: (msg: { id: string; data: string }) => void }) => {
125+
capturedOnMessage = opts.onMessage ?? null;
126+
return new Promise<void>(() => {});
127+
},
128+
);
104129

105130
const { result } = renderHook(() =>
106131
// biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests
107132
useAnalyticsQuery("test_query" as any),
108133
);
109134

110-
// Initially: loading, no status, no data.
111135
expect(result.current.loading).toBe(true);
112136
expect(result.current.warehouseStatus).toBeNull();
113137
expect(result.current.data).toBeNull();
114138
expect(capturedOnMessage).toBeTruthy();
115139

116-
// Server emits a STARTING status — UI should show progress, still loading.
117140
act(() => {
118141
capturedOnMessage?.({
119142
id: "1",
@@ -133,7 +156,6 @@ describe("useAnalyticsQuery", () => {
133156
expect(result.current.loading).toBe(true);
134157
expect(result.current.data).toBeNull();
135158

136-
// Then RUNNING.
137159
act(() => {
138160
capturedOnMessage?.({
139161
id: "2",
@@ -148,7 +170,6 @@ describe("useAnalyticsQuery", () => {
148170
expect(result.current.warehouseStatus?.state).toBe("RUNNING");
149171
});
150172

151-
// Finally the SQL result lands.
152173
act(() => {
153174
capturedOnMessage?.({
154175
id: "3",
@@ -164,21 +185,19 @@ describe("useAnalyticsQuery", () => {
164185
});
165186
expect(result.current.data).toEqual([{ id: 1, name: "row1" }]);
166187
expect(result.current.error).toBeNull();
167-
// warehouseStatus is left at its last observed value (RUNNING) so
168-
// consumers that gated on `state !== "RUNNING"` flip back to data.
169188
expect(result.current.warehouseStatus?.state).toBe("RUNNING");
170189
});
171190

172191
test("surfaces an error when a warehouse_status event has no status payload", async () => {
173-
// A malformed frame must terminate the stream so the hook doesn't
174-
// stay stuck in `loading: true` after a clean stream close.
175192
let capturedOnMessage:
176193
| ((msg: { id: string; data: string }) => void)
177194
| null = null;
178-
mockConnectSSE.mockImplementationOnce((opts: any) => {
179-
capturedOnMessage = opts.onMessage;
180-
return new Promise<void>(() => {});
181-
});
195+
mockConnectSSE.mockImplementationOnce(
196+
(opts: { onMessage?: (msg: { id: string; data: string }) => void }) => {
197+
capturedOnMessage = opts.onMessage ?? null;
198+
return new Promise<void>(() => {});
199+
},
200+
);
182201

183202
const consoleError = vi
184203
.spyOn(console, "error")
@@ -203,4 +222,68 @@ describe("useAnalyticsQuery", () => {
203222
consoleError.mockRestore();
204223
});
205224
});
225+
226+
describe("aborted controller", () => {
227+
test("ignores late error envelope after the controller was aborted", async () => {
228+
const { result } = renderHook(() =>
229+
// biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests
230+
useAnalyticsQuery("test_query" as any),
231+
);
232+
233+
await waitFor(() => expect(capturedCallbacks.signal).toBeDefined());
234+
235+
markAborted();
236+
237+
await act(async () => {
238+
await capturedCallbacks.onMessage?.({
239+
data: JSON.stringify({
240+
type: "error",
241+
error: "Statement failed: The operation was aborted.",
242+
code: "UPSTREAM_ERROR",
243+
}),
244+
});
245+
});
246+
247+
expect(result.current.error).toBeNull();
248+
});
249+
250+
test("ignores late result envelope after the controller was aborted", async () => {
251+
const { result } = renderHook(() =>
252+
// biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests
253+
useAnalyticsQuery("test_query" as any),
254+
);
255+
256+
await waitFor(() => expect(capturedCallbacks.signal).toBeDefined());
257+
258+
markAborted();
259+
260+
await act(async () => {
261+
await capturedCallbacks.onMessage?.({
262+
data: JSON.stringify({ type: "result", data: [{ id: 99 }] }),
263+
});
264+
});
265+
266+
expect(result.current.data).toBeNull();
267+
});
268+
269+
test("ignores late arrow envelope after the controller was aborted", async () => {
270+
const { result } = renderHook(() =>
271+
useAnalyticsQuery("test_query", null, { format: "ARROW_STREAM" }),
272+
);
273+
274+
await waitFor(() => expect(capturedCallbacks.signal).toBeDefined());
275+
276+
markAborted();
277+
278+
await act(async () => {
279+
await capturedCallbacks.onMessage?.({
280+
data: JSON.stringify({ type: "arrow", statement_id: "stmt-123" }),
281+
});
282+
});
283+
284+
expect(mockFetchArrow).not.toHaveBeenCalled();
285+
expect(result.current.data).toBeNull();
286+
expect(result.current.error).toBeNull();
287+
});
288+
});
206289
});

packages/appkit-ui/src/react/hooks/use-analytics-query.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,9 @@ export function useAnalyticsQuery<
263263
payload,
264264
signal: abortController.signal,
265265
onMessage: async (message) => {
266+
// Drop late envelopes from a stream whose controller was already
267+
// aborted (React StrictMode unmount→remount). Mirrors onError below.
268+
if (abortController.signal.aborted) return;
266269
try {
267270
const parsed = JSON.parse(message.data) as Record<string, unknown>;
268271
await handleAnalyticsSseMessage(parsed, sseContext);

0 commit comments

Comments
 (0)