diff --git a/hub/src/socket/server.ts b/hub/src/socket/server.ts index 8f804e37..02728afa 100644 --- a/hub/src/socket/server.ts +++ b/hub/src/socket/server.ts @@ -9,6 +9,7 @@ import { parseAccessToken } from '../utils/accessToken' import { registerCliHandlers } from './handlers/cli' import { registerTerminalHandlers } from './handlers/terminal' import { RpcRegistry } from './rpcRegistry' +import { SOCKET_MAX_HTTP_BUFFER_SIZE } from './socketLimits' import type { SyncEvent } from '../sync/syncEngine' import { TerminalRegistry } from './terminalRegistry' import type { CliSocketWithData, SocketData, SocketServer } from './socketTypes' @@ -62,12 +63,14 @@ export function createSocketServer(deps: SocketServerDeps): { } const io = new Server({ - cors: corsOptions + cors: corsOptions, + maxHttpBufferSize: SOCKET_MAX_HTTP_BUFFER_SIZE }) const engine = new Engine({ path: '/socket.io/', cors: corsOptions, + maxHttpBufferSize: SOCKET_MAX_HTTP_BUFFER_SIZE, allowRequest: async (req) => { const origin = req.headers.get('origin') if (!origin || allowAllOrigins || corsOrigins.includes(origin)) { diff --git a/hub/src/socket/socketLimits.test.ts b/hub/src/socket/socketLimits.test.ts new file mode 100644 index 00000000..2a8d853d --- /dev/null +++ b/hub/src/socket/socketLimits.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from 'bun:test' +import { MAX_GENERATED_IMAGE_BYTES, SOCKET_MAX_HTTP_BUFFER_SIZE } from './socketLimits' + +describe('socket limits', () => { + it('buffer size can carry the largest generated image after base64 + JSON-RPC framing', () => { + // Generated images cross the /cli socket as a base64 string inside a JSON-RPC envelope. + // base64 inflates the payload by ~4/3; the engine default (1e6) silently drops anything + // above ~750 KB raw (issue #927). The buffer must exceed the largest allowed image. + const base64Bytes = Math.ceil(MAX_GENERATED_IMAGE_BYTES / 3) * 4 + expect(SOCKET_MAX_HTTP_BUFFER_SIZE).toBeGreaterThan(base64Bytes) + // and must be well above the 1 MB engine default that caused the regression + expect(SOCKET_MAX_HTTP_BUFFER_SIZE).toBeGreaterThan(1e6) + }) +}) diff --git a/hub/src/socket/socketLimits.ts b/hub/src/socket/socketLimits.ts new file mode 100644 index 00000000..c578c8c4 --- /dev/null +++ b/hub/src/socket/socketLimits.ts @@ -0,0 +1,10 @@ +// The largest generated image the CLI will serve inline. Must stay in sync with the CLI-side +// limits in cli/src/claude/utils/startHappyServer.ts and cli/src/modules/common/generatedImages.ts. +export const MAX_GENERATED_IMAGE_BYTES = 25 * 1024 * 1024 + +// Generated images (and other large RPC payloads) cross the /cli socket as a base64 string wrapped +// in a JSON-RPC envelope, which inflates the payload by ~4/3. The engine.io default of 1e6 bytes +// silently drops the CLI -> hub ack frame for any image above ~750 KB raw, so a 25 MB image that +// the MCP tool happily accepts can never reach the browser (issue #927). Size the buffer to carry +// the largest allowed image after base64 + framing, with headroom. +export const SOCKET_MAX_HTTP_BUFFER_SIZE = 48 * 1024 * 1024 diff --git a/hub/src/web/routes/git.test.ts b/hub/src/web/routes/git.test.ts new file mode 100644 index 00000000..320f1eb9 --- /dev/null +++ b/hub/src/web/routes/git.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from 'bun:test' +import { Hono } from 'hono' +import type { Session, SyncEngine } from '../../sync/syncEngine' +import type { WebAppEnv } from '../middleware/auth' +import { createGitRoutes } from './git' + +function buildApp(engine: Partial): Hono { + const app = new Hono() + app.use('*', async (c, next) => { + c.set('namespace', 'default') + await next() + }) + app.route('/api', createGitRoutes(() => engine as SyncEngine)) + return app +} + +describe('generated images route', () => { + it('serves generated images with an immutable cache header instead of no-store', async () => { + const pngBytes = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]) + const session = { id: 'session-1', namespace: 'default', active: true } as unknown as Session + const engine = { + resolveSessionAccess: () => ({ ok: true as const, sessionId: 'session-1', session }), + readGeneratedImage: async () => ({ + success: true, + content: pngBytes.toString('base64'), + mimeType: 'image/png', + fileName: 'shot.png' + }) + } as unknown as Partial + + const response = await buildApp(engine).request('/api/sessions/session-1/generated-images/img-1') + + expect(response.status).toBe(200) + const cacheControl = response.headers.get('cache-control') ?? '' + // Generated images are content-addressed by an immutable random id, so they must be + // cacheable; `no-store` forces a full RPC round-trip on every remount (issue #927). + expect(cacheControl).toContain('immutable') + expect(cacheControl).not.toContain('no-store') + expect(response.headers.get('etag')).toBe('"img-1"') + }) + + it('returns 304 without an RPC round-trip when If-None-Match matches', async () => { + const session = { id: 'session-1', namespace: 'default', active: true } as unknown as Session + let rpcCalls = 0 + const engine = { + resolveSessionAccess: () => ({ ok: true as const, sessionId: 'session-1', session }), + readGeneratedImage: async () => { + rpcCalls += 1 + return { success: true, content: '', mimeType: 'image/png', fileName: 'shot.png' } + } + } as unknown as Partial + + const response = await buildApp(engine).request('/api/sessions/session-1/generated-images/img-1', { + headers: { 'if-none-match': '"img-1"' } + }) + + expect(response.status).toBe(304) + // The whole point: a cache hit must not touch the CLI over the socket. + expect(rpcCalls).toBe(0) + }) +}) diff --git a/hub/src/web/routes/git.ts b/hub/src/web/routes/git.ts index 8cd27a1e..08a889e3 100644 --- a/hub/src/web/routes/git.ts +++ b/hub/src/web/routes/git.ts @@ -35,6 +35,21 @@ async function runRpc(fn: () => Promise): Promise { + const trimmed = candidate.trim() + return trimmed === '*' || trimmed.replace(/^W\//, '') === normalized + }) +} + export function createGitRoutes(getSyncEngine: () => SyncEngine | null): Hono { const app = new Hono() @@ -150,16 +165,31 @@ export function createGitRoutes(getSyncEngine: () => SyncEngine | null): Hono engine.readGeneratedImage(sessionResult.sessionId, parsed.data.imageId)) if (!result.success || !result.content) { return c.json({ success: false, error: result.error ?? 'Generated image not found' }, 404) } const bytes = Uint8Array.from(Buffer.from(result.content, 'base64')) + // Generated images are content-addressed by an immutable random id, so the bytes for a + // given id never change. Cache aggressively so remounts/scroll/session reopen don't + // re-run the full HTTP -> socket.io RPC -> base64 round-trip every time (issue #927). return c.body(bytes, 200, { 'Content-Type': result.mimeType ?? 'application/octet-stream', 'Content-Disposition': `inline; filename="${encodeURIComponent(result.fileName ?? 'generated-image')}"`, - 'Cache-Control': 'no-store' + 'Cache-Control': GENERATED_IMAGE_CACHE_CONTROL, + ETag: etag }) }) diff --git a/web/src/hooks/useAuth.test.tsx b/web/src/hooks/useAuth.test.tsx new file mode 100644 index 00000000..a7d7dbc3 --- /dev/null +++ b/web/src/hooks/useAuth.test.tsx @@ -0,0 +1,79 @@ +import { act, renderHook, waitFor } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' + +// Mock the network layer so we can drive token refreshes deterministically. +// The real ApiClient reads the live token via `getToken`, so the mock records the +// constructor options (including onUnauthorized) and hands out incrementing tokens. +const h = vi.hoisted(() => { + let idSeq = 0 + let authCount = 0 + class MockApiClient { + token: string + options: { getToken?: () => string | null; onUnauthorized?: () => unknown; baseUrl?: string } | undefined + readonly id: number + constructor(token: string, options?: MockApiClient['options']) { + this.token = token + this.options = options + this.id = ++idSeq + } + async authenticate(): Promise<{ token: string; user: { id: string } }> { + authCount += 1 + return { token: `token-${authCount}`, user: { id: 'u1' } } + } + } + class MockApiError extends Error { + status: number + code?: string + constructor(message: string, status = 401, code?: string) { + super(message) + this.status = status + this.code = code + } + } + return { MockApiClient, MockApiError } +}) + +vi.mock('@/api/client', () => ({ ApiClient: h.MockApiClient, ApiError: h.MockApiError })) + +// Imported after the mock is registered (vi.mock is hoisted). +import { useAuth } from '@/hooks/useAuth' + +type ApiWithOptions = { + id: number + options?: { getToken?: () => string | null; onUnauthorized?: () => unknown } +} + +describe('useAuth — api identity stability across token refresh (issue #927)', () => { + it('keeps the same ApiClient instance when the token refreshes', async () => { + // Stable authSource reference, exactly like the real caller (useAuthSource holds it in + // useState). This isolates the bug under test: a *token* refresh, not a source change. + const authSource = { type: 'accessToken' as const, token: 'seed' } + const { result } = renderHook(() => useAuth(authSource, 'http://hub.test')) + + // Initial authenticate resolves and sets the first token. + await waitFor(() => expect(result.current.api).not.toBeNull()) + const api1 = result.current.api as unknown as ApiWithOptions + const token1 = result.current.token + expect(token1).toBe('token-1') + + // Drive the exact real-world trigger: a 401 invokes onUnauthorized, + // which force-refreshes the token (this is what the flaky remote network does). + await act(async () => { + await api1.options?.onUnauthorized?.() + }) + + // The token did advance... + expect(result.current.token).toBe('token-2') + expect(result.current.token).not.toBe(token1) + + // ...but recreating the client was unnecessary: the OLD instance already serves + // the fresh token via getToken, so nothing downstream needed a new `api` reference. + expect(api1.options?.getToken?.()).toBe(result.current.token) + + // DESIRED: `api` stays referentially stable across a refresh, so effects keyed on + // `api` (VoiceBackendSession `[props.api]`, GeneratedImageCard `[ctx.api, ...]`) do + // NOT re-run / remount. On current code `api` is rebuilt because `token` is a useMemo + // dep, which drives the Voice-remount spam + per-image refetch storm. This fails today. + expect(result.current.api).toBe(api1 as unknown as typeof result.current.api) + }) +}) diff --git a/web/src/hooks/useAuth.ts b/web/src/hooks/useAuth.ts index 054ced5a..8a6c38b2 100644 --- a/web/src/hooks/useAuth.ts +++ b/web/src/hooks/useAuth.ts @@ -157,15 +157,21 @@ export function useAuth(authSource: AuthSource | null, baseUrl: string): { } }, [baseUrl]) + // Keep the ApiClient referentially stable across token *refreshes*: the client always reads + // the live token via getToken (tokenRef), so it never needs rebuilding when the token value + // changes — only when auth presence toggles (login/logout). Rebuilding on every refresh churns + // `api`'s identity, which remounts everything keyed on it (VoiceBackendSession `[props.api]`, + // GeneratedImageCard `[ctx.api, ...]`) and drives the remount/refetch storm. Issue #927. + const hasToken = token !== null const api = useMemo(() => ( - token - ? new ApiClient(token, { + hasToken + ? new ApiClient(tokenRef.current ?? '', { baseUrl, getToken: () => tokenRef.current, onUnauthorized: () => refreshAuth({ force: true }) }) : null - ), [baseUrl, refreshAuth, token]) + ), [baseUrl, refreshAuth, hasToken]) useEffect(() => { let isCancelled = false