mirror of
https://github.com/block/buzz.git
synced 2026-10-29 21:41:33 +00:00
fix(desktop): signal-free roster attribution gate; seed the frame-gap guard
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 <noreply@anthropic.com>
Signed-off-by: Max Lampert <maxwell@squareup.com>
This commit is contained in:
@@ -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;
|
||||
},
|
||||
|
||||
@@ -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 = {}) {
|
||||
|
||||
@@ -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<string, number>();
|
||||
|
||||
/** 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.
|
||||
|
||||
Reference in New Issue
Block a user