From ad981ff07cd6a4af67d588ffb6759760544a5708 Mon Sep 17 00:00:00 2001 From: Max Lampert Date: Tue, 25 Aug 2026 00:41:27 -0700 Subject: [PATCH] fix(desktop): signal-free roster attribution gate; seed the frame-gap guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blind-review round 12 (Opus xhigh) findings, both reproduced: - P1: the round-9 abort gate destructured { signal } in the members queryFn — merely reading that getter sets React Query's abortSignalConsumed, switching the roster query to cancel-and-revert when its last observer unsubscribes mid-fetch. Interrupted switches discarded rosters that previously landed in cache (the Tauri call cannot be cancelled, so the work was paid and thrown away), and A->B->A warm switches repaid a full roster fetch. Replaced with a per-channel supersession token (openChannelMembersFetch) checked by traceChannelMembersFetch — stale fetches stay out of the one-shot slot without touching the signal. Sequences reset with community state. - P2: lastFrameAt started null, so the settle-entry -> first-frame window skipped the starvation guard: a no-visibilitychange suspension there recorded a truncated measure inflated by the whole stall (bounded only by the 35s age guard). The gap clock is now seeded at settle entry. Co-Authored-By: Claude Fable 5 Signed-off-by: Max Lampert --- desktop/src/features/channels/hooks.ts | 19 ++++--- .../src/shared/lib/channelSwitchPerf.test.mjs | 53 +++++++++++++++++++ desktop/src/shared/lib/channelSwitchPerf.ts | 32 ++++++++++- 3 files changed, 96 insertions(+), 8 deletions(-) diff --git a/desktop/src/features/channels/hooks.ts b/desktop/src/features/channels/hooks.ts index 7490873e2a..beef0614a6 100644 --- a/desktop/src/features/channels/hooks.ts +++ b/desktop/src/features/channels/hooks.ts @@ -42,7 +42,10 @@ import type { } from "@/shared/api/tauriChannels"; import { mergeConcurrentChannelRecency } from "@/features/channels/lib/channelRecencyMerge"; import { useIdentityQuery } from "@/shared/api/hooks"; -import { traceChannelMembersFetch } from "@/shared/lib/channelSwitchPerf"; +import { + openChannelMembersFetch, + traceChannelMembersFetch, +} from "@/shared/lib/channelSwitchPerf"; import { useFocusedRefetchInterval } from "@/shared/lib/useDocumentVisible"; import { useCommunities } from "@/features/communities/useCommunities"; import { @@ -560,23 +563,25 @@ export function useChannelMembersQuery( return useQuery({ enabled: enabled && channelId !== null, queryKey: ["channels", channelId ?? "none", "members"], - queryFn: async ({ signal }) => { + queryFn: async () => { if (!channelId) { throw new Error("No channel selected."); } + // Supersession token, NOT the query's AbortSignal: reading the signal + // getter flips React Query to cancel-and-revert when the last observer + // unsubscribes mid-fetch, which would discard warm rosters on + // interrupted switches. The token keeps stale fetches (replaced by a + // live join/leave invalidation) out of the trace's one-shot slot. + const fetchAttempt = openChannelMembersFetch(channelId); const fetchStartedAt = performance.now(); const members = await getChannelMembers(channelId); - // Attribute only accepted fetches: a live join/leave invalidation - // cancels and replaces an in-flight roster refetch, and the superseded - // fetch must not claim the trace's one-shot membersFetch slot with a - // stale count — same rule as the window fetch's reconcile abort gate. - signal.throwIfAborted(); traceChannelMembersFetch( channelId, members.length, performance.now() - fetchStartedAt, fetchStartedAt, + fetchAttempt, ); return members; }, diff --git a/desktop/src/shared/lib/channelSwitchPerf.test.mjs b/desktop/src/shared/lib/channelSwitchPerf.test.mjs index 3b0c24909a..307420ed37 100644 --- a/desktop/src/shared/lib/channelSwitchPerf.test.mjs +++ b/desktop/src/shared/lib/channelSwitchPerf.test.mjs @@ -186,6 +186,59 @@ test("fetches attribute only when started after the switch began", () => { assert.equal(shouldAttributeFetch(null, "abcdef1234567890", 1_500), false); }); +test("a superseded members fetch never claims the trace's one-shot slot", async () => { + const { openChannelMembersFetch, traceChannelMembersFetch } = await import( + "./channelSwitchPerf.ts" + ); + await withSettleHarness(async ({ begin, settle, flush }) => { + begin("aaaa1111aaaa1111"); + const startedAt = performance.now(); + // Fetch #1 starts, then a live join/leave invalidation replaces it with + // fetch #2. #1 resolves first (the Tauri call can't be cancelled) but + // must not attribute: its roster is not the one rendered. The query's + // AbortSignal is deliberately not used for this — consuming it flips + // React Query to cancel-and-revert on last-observer unsubscribe, which + // discards warm rosters on interrupted switches. + const first = openChannelMembersFetch("aaaa1111aaaa1111"); + const second = openChannelMembersFetch("aaaa1111aaaa1111"); + traceChannelMembersFetch("aaaa1111aaaa1111", 9_999, 900, startedAt, first); + traceChannelMembersFetch( + "aaaa1111aaaa1111", + 10_002, + 120, + startedAt, + second, + ); + settle("aaaa1111aaaa1111"); + flush(); + const measure = performance + .getEntriesByName("buzz:channel-switch:click-to-settled") + .at(-1); + assert.equal(measure?.detail?.membersFetch?.memberCount, 10_002); + assert.equal(measure?.detail?.membersFetch?.durationMs, 120); + }); +}); + +test("a suspension before the first settle frame drops, not records truncated", async () => { + await withSettleHarness(async ({ begin, settle, flush, measures }) => { + const virtualClock = { now: 0 }; + performance.now = () => virtualClock.now; + try { + // The deferred marker stays latched during a suspension, so its truth + // is not evidence of slow rendering — the settle-entry → first-frame + // window must be starvation-guarded like every later frame. + globalThis.document.querySelector = () => ({}); + begin("aaaa1111aaaa1111"); + settle("aaaa1111aaaa1111"); + virtualClock.now = 20_000; + flush(); + assert.deepEqual(measures(), []); + } finally { + delete performance.now; + } + }); +}); + // --- Settle lifecycle: rapid switches and community resets ---------------- async function withSettleHarness(run, documentOverrides = {}) { diff --git a/desktop/src/shared/lib/channelSwitchPerf.ts b/desktop/src/shared/lib/channelSwitchPerf.ts index 0ecb4778b6..94322bfcbf 100644 --- a/desktop/src/shared/lib/channelSwitchPerf.ts +++ b/desktop/src/shared/lib/channelSwitchPerf.ts @@ -300,12 +300,38 @@ export function traceChannelWindowFetch( activeTrace.windowFetch ??= { durationMs, eventCount }; } +/** + * Per-channel roster-fetch sequence numbers. A live join/leave invalidation + * cancels-and-replaces an in-flight roster refetch, but the underlying + * Tauri call cannot be cancelled — the superseded fetch still resolves and + * must not claim the trace's one-shot slot with a stale count. The query's + * AbortSignal is deliberately NOT used for this: merely reading + * `context.signal` flips React Query to cancel-and-revert when the last + * observer unsubscribes mid-fetch, discarding warm rosters on interrupted + * switches (a product-behavior change this instrumentation must not make). + */ +const channelMembersFetchSequences = new Map(); + +/** Registers a roster fetch attempt; pass the token to traceChannelMembersFetch. */ +export function openChannelMembersFetch(channelId: string): number { + const next = (channelMembersFetchSequences.get(channelId) ?? 0) + 1; + channelMembersFetchSequences.set(channelId, next); + return next; +} + export function traceChannelMembersFetch( channelId: string, memberCount: number, durationMs: number, fetchStartedAt: number, + fetchAttempt?: number, ): void { + if ( + fetchAttempt !== undefined && + channelMembersFetchSequences.get(channelId) !== fetchAttempt + ) { + return; + } if (!shouldAttributeFetch(activeTrace, channelId, fetchStartedAt)) return; activeTrace.membersFetch ??= { durationMs, memberCount }; } @@ -319,6 +345,7 @@ export function traceChannelMembersFetch( export function resetChannelSwitchTrace(): void { activeTrace = null; pendingRouteExitAbandons.clear(); + channelMembersFetchSequences.clear(); } /** Bound on waiting for the deferred timeline commit before recording. */ @@ -440,7 +467,10 @@ export function settleChannelSwitchTrace(channelId: string): void { const dropTrace = () => { if (activeTrace === trace) activeTrace = null; }; - let lastFrameAt: number | null = null; + // Seeded now, not on the first frame: the settle-entry → first-frame + // window must be starvation-guarded too, or a suspension there records a + // truncated measure inflated by the whole stall. + let lastFrameAt: number | null = performance.now(); const awaitDeferredCommit = () => { if (activeTrace !== trace) { // A newer switch replaced this trace, or a community reset dropped it.