From 78155a9d277b87f849fa9ad8ea9f7657ea9c4614 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 18 Jun 2026 03:13:36 +0100 Subject: [PATCH] perf(web): add staleTime to useSession to suppress focus/mount refetches (#884) (#885) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(web): suppress useSession refetch storm (closes #884) Two compounding behaviours in the React client were producing a sustained ~100 req/sec stream of GET /api/sessions/ to the hub on installs with a moderately-sized session fleet: 1. `useSession` had no `staleTime`, so window-focus and remount each triggered a fresh REST round-trip even though SSE was already pushing the same data via `patchSessionDetail`. 2. `useSSE`'s `session-added`/`session-updated` handler unconditionally queued a per-session `invalidateQueries({queryKey: ['session', id]})` whenever the incoming SSE payload was not a structured patch, fanning out a refetch to every still-observed `useSession` regardless of whether the user was currently viewing that session detail. Fixes: - `useSession` now sets `staleTime: 30_000` (exported as `SESSION_DETAIL_STALE_TIME_MS` for testability/tuning). SSE remains authoritative for freshness; the REST endpoint is a cold-start path. - `useSSE` only queues per-session detail invalidation when an active observer is mounted for that session. The new `hasActiveSessionDetailObserver` helper checks the TanStack query cache for `getObserversCount() > 0`. List-summary invalidation is unchanged (sidebar still updates). No behaviour change for the patch-path: structured `SessionPatch` events still flow through `patchSessionDetail` + `patchSessionSummary` and update in place. The fallback path is what we are taming. Measured on the reporter's box pre-fix: 31,944 GET /api/sessions/ hits over a 5-minute idle window across 132 distinct session UUIDs (~106 req/sec). Expected post-fix: ~0 for sessions whose detail page is not currently open, gated by `staleTime` for navigation thrash. Tests: - `useSession.test.ts` asserts `SESSION_DETAIL_STALE_TIME_MS` is set. - `useSSE.test.ts` adds 4 cases for `hasActiveSessionDetailObserver` covering no-cache, cache-without-observer, mounted-observer, and cross-session isolation. * fix(web): revert observer-gating in useSSE (address PR #885 review) The Codex review on #885 correctly flagged that `hasActiveSessionDetailObserver`-gating around the two `queueSessionDetailInvalidation` fallback paths broke an important correctness invariant: with `staleTime: 30_000` in place, skipping the invalidation entirely (instead of letting TanStack mark the cache stale) means a subsequent remount within 30s will serve the stale cached detail without a REST recovery fetch. This regressed real backend code paths. Hub emits `session-updated` events with no structured `data` field on todos / teamState / metadata / agentState changes (see `hub/src/socket/handlers/cli/sessionHandlers.ts:117,128,216,263`), which hit the gated `else` branch. Root cause of the over-correction was a misunderstanding of TanStack v5 semantics: `invalidateQueries` with the default `refetchType: 'active'` is *already* a network no-op for unobserved queries — it just marks them stale. The manual observer-count check was structurally redundant *and* incorrectly suppressed the stale marking. Revert: restore the original unconditional `queueSessionDetailInvalidation` calls on both fallback branches. Drop the `hasActiveSessionDetailObserver` helper export and its 4 unit-test cases. Keep Fix A (`staleTime: 30_000` on `useSession`) intact — that change is independently safe and addresses the focus-refetch / remount-refetch class of redundant requests. * docs(web): correct staleTime rationale in useSession (#884) Stand-in cold review on PR #885 caught that the comment overstated the fix's reach. `web/src/lib/query-client.ts:7` already sets the global default `refetchOnWindowFocus: false` and `staleTime: 5_000`, so the per-query `staleTime: 30_000` does NOT cut focus-refetches (there were none) and only extends the remount/reconnect-no-refetch window from 5s to 30s. Rewrite the comment to be accurate about scope: the change suppresses remount refetches within a 30s window, and explicit `invalidateQueries` (SSE fallback path, reconnect-recovery in `App.tsx`) still refetches active observers — so live updates and recovery flows are preserved. No code behaviour change; comment-only edit. * fix(web): invalidate all cached session details on SSE reconnect Codex review on PR #885 caught a real regression introduced by the `SESSION_DETAIL_STALE_TIME_MS = 30_000` change: the reconnect-recovery handler in `App.tsx` only invalidated the *currently-selected* session's detail. With per-query staleTime extended from 5s (global default) to 30s, a previously-viewed but non-selected session whose cache was still within the freshness window could serve stale data after the SSE channel missed updates during the disconnect. Scenario: 1. User views session A → cache populated, fresh. 2. User switches to session B → A's observer unmounts, cache lingers (gcTime: 5min). 3. SSE disconnects. Session A receives updates server-side that no patch event reaches the client. 4. SSE reconnects. Old `handleSseConnect` only invalidated `session(selectedSessionId=B)`, NOT A. 5. User navigates back to A within 30s → useSession remounts → cache is still considered fresh by staleTime → no REST recovery fetch → user sees stale A data. Fix: broaden the per-session invalidation in `handleSseConnect` from `['session', selectedSessionId]` to the prefix `['session']`, which matches every cached session-detail entry. Active observers refetch (same as before — only the selected session was active), inactive cached entries get marked stale so the next remount refetches. Performance impact: zero new fetches on reconnect (the selected session is still the only one with an active observer in practice). Marking inactive entries stale is metadata-only, free. This restores the pre-staleTime invariant where every cached session detail was either fresh (just fetched) or actively re-fetched on reconnect, and matches the documented contract that SSE is the authoritative freshness signal while REST is the cold-start / reconnect-recovery path. --------- Co-authored-by: heavygee --- web/src/App.tsx | 9 ++++++--- web/src/hooks/queries/useSession.test.ts | 12 +++++++++++- web/src/hooks/queries/useSession.ts | 12 ++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/web/src/App.tsx b/web/src/App.tsx index fc23635a..3a11e6c2 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -205,9 +205,12 @@ function AppInner() { } const invalidations = [ queryClient.invalidateQueries({ queryKey: queryKeys.sessions }), - ...(selectedSessionId ? [ - queryClient.invalidateQueries({ queryKey: queryKeys.session(selectedSessionId) }) - ] : []) + // Invalidate ALL cached session-detail entries on reconnect, not just + // the selected one. With `SESSION_DETAIL_STALE_TIME_MS` extending the + // freshness window on `useSession`, a previously-viewed session that + // received updates during the SSE gap would otherwise serve stale + // cached data on remount. See tiann/hapi#884. + queryClient.invalidateQueries({ queryKey: ['session'] }) ] const refreshMessages = (selectedSessionId && api) ? fetchLatestMessages(api, selectedSessionId) diff --git a/web/src/hooks/queries/useSession.test.ts b/web/src/hooks/queries/useSession.test.ts index b5adcfdc..7d374cbc 100644 --- a/web/src/hooks/queries/useSession.test.ts +++ b/web/src/hooks/queries/useSession.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { isSessionNotFoundError } from './useSession' +import { isSessionNotFoundError, SESSION_DETAIL_STALE_TIME_MS } from './useSession' describe('isSessionNotFoundError', () => { it('matches hub 404 session responses', () => { @@ -11,3 +11,13 @@ describe('isSessionNotFoundError', () => { expect(isSessionNotFoundError(null)).toBe(false) }) }) + +describe('SESSION_DETAIL_STALE_TIME_MS', () => { + // SSE patches the cache directly on session-updated events, so the REST + // endpoint is just a cold-start / reconnect-recovery path. A long staleTime + // suppresses focus-refetch and remount-refetch storms — primary lever for + // the refetch-storm fix (tiann/hapi#884). + it('is set to a value that suppresses focus/mount refetches', () => { + expect(SESSION_DETAIL_STALE_TIME_MS).toBeGreaterThanOrEqual(10_000) + }) +}) diff --git a/web/src/hooks/queries/useSession.ts b/web/src/hooks/queries/useSession.ts index d9d6e5be..adf086ea 100644 --- a/web/src/hooks/queries/useSession.ts +++ b/web/src/hooks/queries/useSession.ts @@ -8,6 +8,17 @@ export function isSessionNotFoundError(error: unknown): boolean { && (error.message.includes('HTTP 404') || error.message.includes('Session not found')) } +// Session detail freshness is driven by SSE events (`useSSE` patches the cache +// directly on `session-updated`). The REST endpoint is only a cold-start / +// reconnect-recovery path, so a long per-query staleTime extends the global +// default (5s, see `web/src/lib/query-client.ts`) for `useSession` only — this +// suppresses remount-refetch when the user navigates back to a recently-viewed +// session within the window, without making the UI stale. Explicit +// `invalidateQueries` calls (SSE fallback path, reconnect-recovery in +// `App.tsx`) still refetch active observers regardless of staleTime, so live +// updates and recovery flows continue to work. See tiann/hapi#884. +export const SESSION_DETAIL_STALE_TIME_MS = 30_000 + export function useSession(api: ApiClient | null, sessionId: string | null): { session: Session | null isLoading: boolean @@ -25,6 +36,7 @@ export function useSession(api: ApiClient | null, sessionId: string | null): { return await api.getSession(sessionId) }, enabled: Boolean(api && sessionId), + staleTime: SESSION_DETAIL_STALE_TIME_MS, retry: (failureCount, error) => { if (isSessionNotFoundError(error)) { return false