mirror of
https://github.com/wu736139669/hapi.git
synced 2026-08-05 06:24:37 +00:00
* 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/<uuid> 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/<uuid> 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 <heavygee@users.noreply.github.com>
This commit is contained in:
+6
-3
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user