fix(desktop): harden profile batch and thread-reply fetches against relay slowness (#7188)

Three client gaps turn transient relay failures into permanent UI
degradation. Under a slow or rate-limited relay:

1. A cold channel's profile batch exhausts its single retry and leaves
raw npubs + broken mention chips until the user manually kicks the
channel.
2. A thread opened from a notification trusts a successful-but-empty
reply read as authoritative and never retries.
3. A rate-limited `CLOSED` on a history subscription immediately rejects
the caller rather than retrying after the rate-limit window.

All three are addressed without changing global query defaults or the
happy-path behavior.

## Changes

**Fix 1 — cold profile batch resilience** (`useUsersBatchQuery`,
`desktop/src/features/profile/hooks.ts`)

Override `retry: 3` with exponential backoff and error-gated
`refetchOnWindowFocus: (query) => query.state.status === "error"`,
scoped to this query only. The global defaults (`retry: 1`,
`refetchOnWindowFocus: false`) are intentional for other queries and are
unchanged. After the retry budget exhausts, a window-focus event (e.g.
channel-switch) recovers the query automatically — but only when it is
already in an error state, preventing unnecessary refetches for
successful batches.

**Fix 2 — stale-empty thread reads** (`useThreadReplies.ts`,
`ChannelScreen.tsx`)

Add optional `expectedEventId` parameter. When a completed paged fetch
does not contain the expected event, throw
`ThreadExpectedEventMissingError` so React Query's built-in retry
machinery handles it rather than caching an authoritative empty.
`ChannelScreen` passes `threadScrollTargetId` (the notification-linked
reply ID) as `expectedEventId`.

When notification routing changes `expectedEventId` while the same
thread root is already mounted (same query key), an explicit
`invalidateQueries` in a `useEffect` triggers a fresh validation pass.
The `useEffect` is declared after `useQuery` so TanStack's internal
options-update effect installs the new `queryFn` closure first; the
refetch therefore uses the current `expectedEventId` rather than the
previous null closure. For the cold-start race (target arrives before
the first page returns), the effect detects `fetchStatus === "fetching"
&& status === "pending"` and calls
`cancelQueries().then(invalidateQueries)` so the obsolete in-flight
response cannot settle as authoritative before the new target's
validation closure is active.

The query-fn tracks consecutive fetch attempts per target. On attempt 3,
it adds the target to `exhaustedTargetsRef` before calling
`loadThreadReplies`. `loadThreadReplies` sees the target in the
exhausted set and returns the fetched replies directly rather than
throwing — the terminal attempt always resolves to success. No
re-entrant scheduling: the resolution is synchronous inside the query
function itself. Deleted/moderated targets never lock the thread in a
terminal error surface.

**Fix 3 — CLOSED recovery for history subscriptions**
(`relayClosedRecovery.ts`, `relayClientSession.ts`,
`relayClientShared.ts`, `relayGateBoundary.ts`)

On a rate-limited `CLOSED` the subscription previously rejected the
caller immediately. Store `filter` and `timeoutMs` on
`HistorySubscription`, then on rate-limited `CLOSED` re-register under a
fresh `subId` and defer `sendReq` until the rate-limit window clears —
matching the live-sub recovery design already present in
`relayClosedRecovery.ts`. Bounded to 3 attempts; exhausted retries
reject immediately so callers are never left waiting indefinitely. A new
op-timeout guards the retry REQ against a non-responding relay; when the
op-timeout fires it sends `CLOSE` for the rotated `subId` (matching the
behavior of the original timeout path) so the relay releases the slot
rather than counting it against the per-connection cap.

## Tests

- `relayClosedRecovery.test.mjs`: behavioral fake-clock tests for
history-sub retry, 3-attempt exhaustion, op-timeout CLOSE send +
late-EOSE non-regression, rejecting-`closeSubscription` swallowed
without unhandled rejection, wiring source assertion (fails if
`relayClientSession.ts` drops the `closeSubscription` callback) — 18
tests
- `useThreadReplies.test.mjs`: `loadThreadReplies` unit tests
(throw/exhaustion-guard); behavioral hook tests via real
`QueryClientProvider` + `renderHook`: exhaustion-resolves-to-data,
settled null→target change retries on missing-target page and lands
target data (fails if `invalidateQueries` is removed OR if `useQuery` is
moved after the `useEffect`), cold-fetch cancel-then-invalidate (gated
fetcher — released after rerender, stale empty discarded, replacement
fetch settles with target); ChannelScreen wiring source assertion — 9
tests
- `profileBatchResilience.test.mjs`: source assertions for `retry: 3`,
`retryDelay`, error-gated `refetchOnWindowFocus`, and unchanged global
defaults — 2 tests

---------

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
This commit is contained in:
Will Pfleger
2026-09-02 17:42:45 -04:00
committed by GitHub
parent 187df22252
commit 6f6093243e
10 changed files with 1209 additions and 23 deletions

View File

@@ -799,7 +799,6 @@ export class RelayClient {
this.handleEose(rest[0], generation);
return;
}
if (type === "CLOSED" && typeof rest[0] === "string") {
handleRelayClosed({
subscriptions: this.subscriptions,
@@ -810,6 +809,7 @@ export class RelayClient {
["REQ", subId, filter],
"Failed to restore relay subscription after CLOSED.",
),
closeSubscription: (subId) => this.closeSubscription(subId),
});
return;
}

View File

@@ -49,10 +49,13 @@ export type RelaySubscriptionFilter = {
type HistorySubscription = {
mode: "history";
filter: RelaySubscriptionFilter;
events: RelayEvent[];
resolve: (events: RelayEvent[]) => void;
reject: (error: Error) => void;
timeout: number;
timeoutMs: number;
closedRetryAttempt?: number;
};
type FirstEventSubscription = {

View File

@@ -60,12 +60,10 @@ function resetAll(startMs = 0) {
resetRateLimitGate();
}
test("production CLOSED handler rejects history once and clears its timeout", () => {
test("production CLOSED handler rejects non-rate-limited history immediately and clears its timeout", () => {
const originalWindow = globalThis.window;
const clearedTimeouts = [];
globalThis.window = {
// Provide both setTimeout (needed by activateRateLimit in the F1 fix) and
// clearTimeout (the existing assertion target).
setTimeout: (_fn, _ms) => 0,
clearTimeout: (timeout) => clearedTimeouts.push(timeout),
};
@@ -76,10 +74,12 @@ test("production CLOSED handler rejects history once and clears its timeout", ()
"history-1",
{
mode: "history",
filter: { kinds: [0], authors: ["pubkey-1"], limit: 10 },
events: [],
resolve: () => assert.fail("CLOSED must not resolve history"),
reject: (error) => errors.push(error),
timeout: 42,
timeoutMs: 25_000,
},
],
]);
@@ -88,23 +88,201 @@ test("production CLOSED handler rejects history once and clears its timeout", ()
subId: "history-1",
sendReq: () => Promise.resolve(),
};
handleRelayClosed({
...input,
message: "rate-limited: too many concurrent requests",
});
// Non-rate-limited CLOSED should reject immediately.
handleRelayClosed({ ...input, message: "error: database unavailable" });
handleRelayClosed({ ...input, message: "late CLOSED" });
assert.equal(subscriptions.has("history-1"), false);
assert.deepEqual(clearedTimeouts, [42]);
assert.equal(errors.length, 1);
assert.equal(
errors[0].message,
"rate-limited: too many concurrent requests",
);
assert.equal(errors[0].message, "error: database unavailable");
} finally {
globalThis.window = originalWindow;
}
});
test("rate-limited history CLOSED schedules retry instead of rejecting immediately", () => {
resetAll(0);
const errors = [];
const reqsSent = [];
const subscriptions = new Map([
[
"history-rl",
{
mode: "history",
filter: { kinds: [0], authors: ["pubkey-1"], limit: 10 },
events: [],
resolve: () => assert.fail("must not resolve before retry"),
reject: (error) => errors.push(error),
timeout: 99,
timeoutMs: 25_000,
},
],
]);
handleRelayClosed({
subscriptions,
subId: "history-rl",
message: "rate-limited: quota exceeded; retry in 5s",
sendReq: (id, filter) => {
reqsSent.push({ id, filter });
return Promise.resolve();
},
});
// Subscription should NOT have been rejected immediately.
assert.equal(
errors.length,
0,
"must not reject immediately on first attempt",
);
// Original subId evicted, a new one registered.
assert.equal(subscriptions.has("history-rl"), false);
// No retry fired yet at t=0.
assert.equal(reqsSent.length, 0, "retry must not fire before delay");
// Fire the retry timer (hint=5s → delayMs=5000).
tickTo(5_001);
assert.equal(
reqsSent.length,
1,
"retry REQ must fire after rate-limit window",
);
});
test("rate-limited history CLOSED exhausts 3 retries then rejects permanently", () => {
resetAll(0);
const errors = [];
const reqsSent = [];
const filter = { kinds: [0], authors: ["pk"], limit: 1 };
const subscription = {
mode: "history",
filter,
events: [],
resolve: () => assert.fail("must not resolve"),
reject: (error) => errors.push(error),
timeout: 0,
timeoutMs: 25_000,
};
const subscriptions = new Map([["history-rl", subscription]]);
const sendReq = (id, _filter) => {
reqsSent.push(id);
return Promise.resolve();
};
// Simulate 3 rate-limited CLOSEDs (exhausts the attempt budget).
for (let i = 0; i < 3; i++) {
// The current live subId rotates on each retry; find it.
const currentSubId = [...subscriptions.keys()].find((k) =>
k.startsWith("history"),
);
assert.ok(currentSubId, `must have a live history sub on attempt ${i}`);
handleRelayClosed({
subscriptions,
subId: currentSubId,
message: "rate-limited: quota exceeded; retry in 1s",
sendReq,
});
assert.equal(
errors.length,
0,
`must not reject before attempt ${i + 1} fires`,
);
tickTo((i + 1) * 1_001);
assert.equal(reqsSent.length, i + 1, `sendReq call ${i + 1} must fire`);
}
// 4th CLOSED — attempt budget exhausted → permanent reject.
const currentSubId = [...subscriptions.keys()].find((k) =>
k.startsWith("history"),
);
assert.ok(
currentSubId,
"must still have a live history sub before last CLOSED",
);
handleRelayClosed({
subscriptions,
subId: currentSubId,
message: "rate-limited: quota exceeded; retry in 1s",
sendReq,
});
assert.equal(errors.length, 1, "must reject after 3 failed retries");
assert.equal(subscriptions.size, 0, "must evict sub after permanent reject");
});
test("rate-limited history retry op-timeout sends CLOSE and rejects without leaking the sub", () => {
// Regression for: retry-path op-timeout deleted the sub and rejected but
// never sent CLOSE for the rotated subId — leaving the relay holding a slot
// against its per-connection cap.
//
// Also validates the late-EOSE non-regression: once the sub is evicted and
// CLOSE sent, a late EOSE cannot double-resolve the promise because
// handleSubscriptionEose guards on subscriptions.has(subId).
resetAll(0);
const errors = [];
const closedSubIds = [];
const filter = { kinds: [9], "#h": ["ch-close"], limit: 50 };
const subscription = {
mode: "history",
filter,
events: [],
resolve: () => assert.fail("must not resolve"),
reject: (error) => errors.push(error),
timeout: 0,
timeoutMs: 5_000,
};
const subscriptions = new Map([["history-close", subscription]]);
// Trigger a rate-limited CLOSED — subscription rotates to a new subId.
handleRelayClosed({
subscriptions,
subId: "history-close",
message: "rate-limited: quota exceeded; retry in 1s",
sendReq: () => Promise.resolve(),
closeSubscription: async (id) => {
closedSubIds.push(id);
},
});
assert.equal(errors.length, 0, "must not reject before delay fires");
// Find the rotated subId.
const newSubId = [...subscriptions.keys()].find((k) =>
k.startsWith("history"),
);
assert.ok(newSubId, "rotated sub must be registered");
assert.notEqual(newSubId, "history-close", "sub must have been rotated");
// The retry-delay timer fires: sendReq runs and arms a new op-timeout.
tickTo(1_001);
assert.equal(errors.length, 0, "still must not reject immediately after REQ");
// The op-timeout fires (delay + timeoutMs = 1001 + 5000 = 6001 ms).
tickTo(6_002);
assert.equal(
errors.length,
1,
"must reject after op-timeout on the retried sub",
);
assert.ok(
closedSubIds.includes(newSubId),
`CLOSE must be sent for the rotated sub ${newSubId} (got: ${JSON.stringify(closedSubIds)})`,
);
assert.equal(
subscriptions.has(newSubId),
false,
"rotated sub must be evicted after op-timeout",
);
// Late-EOSE non-regression: arriving after eviction must be a no-op.
const prevErrors = errors.length;
handleSubscriptionEose({
subscriptions,
subId: newSubId,
closeSubscription: async () => {},
});
assert.equal(
errors.length,
prevErrors,
"late EOSE after eviction must not alter error state",
);
});
test("rate-limited history CLOSED arms the shared gate for concurrent ops", () => {
resetAll(0);
const subscriptions = new Map([
@@ -112,10 +290,12 @@ test("rate-limited history CLOSED arms the shared gate for concurrent ops", () =
"history-1",
{
mode: "history",
filter: { kinds: [0], authors: ["pubkey-1"], limit: 10 },
events: [],
resolve: () => {},
reject: () => {},
timeout: 0,
timeoutMs: 25_000,
},
],
]);
@@ -142,10 +322,12 @@ test("non-rate-limited history CLOSED does not arm the gate", () => {
"history-2",
{
mode: "history",
filter: { kinds: [9], "#h": ["ch-1"], limit: 50 },
events: [],
resolve: () => {},
reject: () => {},
timeout: 0,
timeoutMs: 25_000,
},
],
]);
@@ -173,10 +355,12 @@ test("gate armed by rate-limited history CLOSED defers the next REQ until expiry
"history-gate",
{
mode: "history",
filter: { kinds: [9], "#h": ["ch-gate"], limit: 50 },
events: [],
resolve: () => {},
reject: () => {},
timeout: 0,
timeoutMs: 25_000,
},
],
]);
@@ -486,6 +670,89 @@ test("terminal CLOSED deletes subscription and does not retry", () => {
assert.equal(firedAt.length, 0, "terminal CLOSED must not retry");
});
// ── Test: rejecting closeSubscription does not produce an unhandled rejection ─
//
// Load-bearing for the `.catch(() => {})` guard on the op-timeout CLOSE send.
//
// When the IPC/socket call inside closeSubscription throws (e.g. the connection
// was already torn down), the rejection must be swallowed — the history promise
// is already being rejected by the line below the CLOSE send. Without the catch,
// a Node/browser unhandled-rejection event fires and surfaces as a test failure.
test("rejecting closeSubscription on op-timeout does not produce an unhandled rejection", async () => {
resetAll(0);
const errors = [];
const filter = { kinds: [9], "#h": ["ch-reject"], limit: 50 };
const subscription = {
mode: "history",
filter,
events: [],
resolve: () => assert.fail("must not resolve"),
reject: (error) => errors.push(error),
timeout: 0,
timeoutMs: 5_000,
};
const subscriptions = new Map([["history-reject", subscription]]);
// closeSubscription that rejects — simulates a broken socket.
handleRelayClosed({
subscriptions,
subId: "history-reject",
message: "rate-limited: quota exceeded; retry in 1s",
sendReq: () => Promise.resolve(),
closeSubscription: async () => {
throw new Error("socket already closed");
},
});
// Fire the retry delay and op-timeout.
tickTo(1_001);
tickTo(6_002);
// Let any async promise chains settle.
await Promise.resolve();
await Promise.resolve();
// The history promise must have been rejected exactly once despite the
// closeSubscription rejection — no unhandled rejection from the CLOSE send.
assert.equal(
errors.length,
1,
"history promise must be rejected exactly once; the CLOSE rejection is swallowed",
);
assert.ok(
errors[0].message.includes("Relay closed"),
`rejection must be the history error, not the CLOSE error (got: ${errors[0].message})`,
);
});
// ── Test: CLOSE wiring at the production call site ────────────────────────────
//
// Falsifiable guard: asserts that relayClientSession.ts passes closeSubscription
// to handleRelayClosed. Removing the wiring line makes this test promptly red —
// closing the gap where the helper behavioral test passes without the actual
// production CLOSE being sent.
test("relayClientSession wires closeSubscription into handleRelayClosed", async () => {
const { readFile } = await import("node:fs/promises");
const source = await readFile(
new URL("./relayClientSession.ts", import.meta.url),
"utf8",
);
// The handleRelayClosed call must pass a closeSubscription callback.
assert.match(
source,
/handleRelayClosed\(\{[^}]*closeSubscription[^}]*\}\)/,
"relayClientSession.ts must pass closeSubscription to handleRelayClosed",
);
// The callback must reach this.closeSubscription so it sends a real CLOSE.
assert.match(
source,
/closeSubscription:\s*\([^)]+\)\s*=>\s*this\.closeSubscription\(/,
"closeSubscription callback must delegate to this.closeSubscription()",
);
});
// ── Teardown ──────────────────────────────────────────────────────────────────
test("teardown — restore Date.now", () => {

View File

@@ -28,23 +28,72 @@ export function handleRelayClosed({
subId,
message,
sendReq,
closeSubscription,
}: {
subscriptions: Map<string, RelaySubscription>;
subId: string;
message: string;
sendReq: (subId: string, filter: RelaySubscriptionFilter) => Promise<void>;
closeSubscription?: (subId: string) => Promise<void>;
}) {
const subscription = subscriptions.get(subId);
if (!subscription) return;
if (subscription.mode !== "live") {
// Classify before rejecting so a `rate-limited:` history CLOSED arms the
// gate for concurrent ops. A history sub can't be retried (the caller holds
// the promise), so we still reject immediately after arming.
// Classify before acting so a `rate-limited:` CLOSED arms the gate for
// concurrent ops regardless of whether this specific sub can be retried.
const closedClass = classifyRelayClosed(message);
if (closedClass === "rate-limited") {
const hintSeconds = parseRateLimitHint(message);
activateRateLimit(hintSeconds);
// History subs hold a promise the caller is awaiting. Rather than
// rejecting immediately, defer and re-issue the REQ after the
// rate-limit window so the caller transparently receives their result.
// Bounded to 3 attempts — if the relay keeps refusing, fall through
// to the permanent reject so the caller's promise resolves with an error
// rather than waiting forever.
if (subscription.mode === "history") {
const attempt = subscription.closedRetryAttempt ?? 0;
if (attempt < 3) {
subscription.closedRetryAttempt = attempt + 1;
// Clear the existing op-timeout so it doesn't fire while waiting for
// the rate-limit window. The setTimeout delay covers this interval.
window.clearTimeout(subscription.timeout);
const hintMs = (hintSeconds ?? 10) * 1_000;
const delayMs = Math.max(rateLimitRemainingMs() || hintMs, hintMs);
// Re-register under a new id so the old subId can be evicted cleanly.
// Accumulated events are preserved on the subscription object.
const newSubId = `history-${crypto.randomUUID()}`;
subscriptions.delete(subId);
subscriptions.set(newSubId, subscription);
// Use the rate-limit delay as the new timeout budget so the
// subscription is not left open indefinitely while waiting.
subscription.timeout = window.setTimeout(() => {
if (!subscriptions.has(newSubId)) return; // cancelled while waiting
void sendReq(newSubId, subscription.filter).catch(() => {
subscriptions.delete(newSubId);
subscription.reject(
new Error(message || "Relay closed the history subscription."),
);
});
// Set a new op-timeout for the retry REQ so the subscription
// cannot hang indefinitely if the relay stops responding.
subscription.timeout = window.setTimeout(() => {
subscriptions.delete(newSubId);
// History promise is already being rejected below; swallow any
// IPC/socket error from the CLOSE send so it does not surface as
// an unhandled rejection on a path that has no live error handler.
closeSubscription?.(newSubId)?.catch(() => {});
subscription.reject(
new Error("Relay closed the history subscription."),
);
}, subscription.timeoutMs);
}, delayMs);
return;
}
}
}
window.clearTimeout(subscription.timeout);
subscriptions.delete(subId);
subscription.reject(

View File

@@ -41,10 +41,12 @@ export async function requestHistoryGated(
subscriptions.set(subId, {
mode: "history",
filter,
events: [],
resolve,
reject,
timeout,
timeoutMs: historyTimeoutMs,
});
void sendRaw(["REQ", subId, filter]).catch((error) => {