Skip to content

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

Open
wpfleger96 wants to merge 1 commit into
mainfrom
duncan/client-resilience
Open

fix(desktop): harden profile batch and thread-reply fetches against relay slowness#7188
wpfleger96 wants to merge 1 commit into
mainfrom
duncan/client-resilience

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Sep 1, 2026

Copy link
Copy Markdown
Member

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. 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, mounted-thread target-change triggers invalidation, 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

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Requesting changes on three P2 correctness issues at 46f545bb1a35d098f7ba79c0369c4958b2743879 (PR merge-base bd73490418266f267d9bb3bdf13e64582adc8e80; current base tip e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f).

The required fixes are bounded: preserve successfully fetched replies when expected-target validation exhausts, validate a target arriving during an existing cold fetch, and close the relay subscription when a retried history request times out. Add regressions through the real hook/transport lifecycle; the copied retry callback and pre-seeded exhausted-set tests do not cover these transitions.

Validation: mounted the production thread hook with real React/TanStack and a stubbed Tauri response; reproduced both thread failures. Exercised the production history request/CLOSED/EOSE handlers with a fake clock and verified the missing CLOSE. Independently traced UI consumers and relay subscription ownership. No new profile-batch blocker found. Broad CI was still running at the snapshot checked; no full-suite rerun or live relay/UI claim. Focus refetch remains disabled despite the PR description, so please align that description with the actual retry/remount behavior.

Comment thread desktop/src/features/messages/useThreadReplies.ts Outdated
Comment thread desktop/src/features/messages/useThreadReplies.ts Outdated
Comment thread desktop/src/shared/api/relayClosedRecovery.ts
@wpfleger96
wpfleger96 force-pushed the duncan/client-resilience branch from 46f545b to b4f65f1 Compare September 1, 2026 18:46
@wpfleger96
wpfleger96 deployed to codex-review September 1, 2026 18:46 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64...0ee208a37f3d5f42ba35093c0774638b5395ae8d.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 0ee208a37f3d5f42ba35093c0774638b5395ae8d to authorize a new review.
Any previous review applies only to its recorded range.

@wpfleger96
wpfleger96 force-pushed the duncan/client-resilience branch from b4f65f1 to e5adb15 Compare September 1, 2026 19:05
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 1, 2026
@wpfleger96
wpfleger96 force-pushed the duncan/client-resilience branch from e5adb15 to 01a6a19 Compare September 1, 2026 19:13
@wpfleger96
wpfleger96 force-pushed the duncan/client-resilience branch from 01a6a19 to 2539566 Compare September 1, 2026 19:20
@wpfleger96
wpfleger96 force-pushed the duncan/client-resilience branch from 2539566 to 71f999b Compare September 1, 2026 20:03
…elay slowness

Under a slow or temporarily rate-limited relay the desktop turns transient
relay failures into permanent degradation: a cold channel's profile batch
exhausts its one retry and leaves raw npubs and broken mention chips forever;
a thread opened from a notification trusts a successful-empty reply read as
authoritative; and a rate-limited CLOSED on a history subscription rejects
immediately rather than retrying after the window clears.

Fix 1 — cold profile batch resilience (useUsersBatchQuery):
Override retry to 3 with exponential backoff, scoped to this query only.
The global defaults (retry: 1, refetchOnWindowFocus: false) are unchanged so
other queries keep their deliberate semantics. refetchOnWindowFocus is NOT
overridden — errored queries auto-refetch on component remount (channel-switch),
which covers the recovery path without firing on every focus event (which breaks
the E2E hover-surface test at message-feedback-snapshots.spec.ts:97).

Fix 2 — stale-empty thread reads (useThreadReplies):
When a completed paged fetch does not contain the expected event (e.g. a reply
opened from a notification), throw ThreadExpectedEventMissingError to engage
React Query's retry-with-backoff rather than caching an authoritative empty.
Add a useEffect to invalidate the query when expectedEventId changes on an
already-mounted thread (same query key — closure does not re-run otherwise).
Track per-target retry exhaustion: after 3 ThreadExpectedEventMissingError
throws for the same target, add it to exhaustedTargets and return the fetched
replies instead of throwing — preventing deleted/moderated targets from locking
the thread in a terminal error state that discards valid replies. Export
loadThreadReplies with an injectable fetcher parameter so behavioral tests can
exercise the real validation logic without a Tauri bridge.

Fix 3 — CLOSED recovery for history subscriptions (relayClosedRecovery):
For rate-limited CLOSED on history subs the relay session previously rejected
the caller's promise immediately. Add store-and-retry: record filter and
timeoutMs on HistorySubscription, then on a rate-limited CLOSED re-register
under a fresh subId and defer sendReq until the rate-limit window clears.
Bounded to 3 attempts; exhausted retries fall through to immediate reject.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants