diff --git a/shared/chat/conversation/thread-message-state.test.tsx b/shared/chat/conversation/thread-message-state.test.tsx index 8ba2bfb62629..ac22c72ee69c 100644 --- a/shared/chat/conversation/thread-message-state.test.tsx +++ b/shared/chat/conversation/thread-message-state.test.tsx @@ -673,3 +673,52 @@ describe('addMessagesToThreadState', () => { expect(merged?.type === 'text' && merged.text.stringValue()).toBe('edited') }) }) + +// The http server can be stopped (BACKGROUND) or mid-restart (a fresh port/token) when an update +// carrying a stale GetURL lands: that comes back as '' or as a base-less URL with only query +// params appended (e.g. "&prev=false&noanim=true"), never as a garbage http:// value. A url +// field only ever takes an incoming value that actually looks like one. +describe('local server urls', () => { + const attachmentOrdinal = T.Chat.numberToOrdinal(201) + const validUrl = 'http://127.0.0.1:1234/at?key=abc' + const garbageUrl = '&prev=false&noanim=true' + + test('a new non-empty url replaces the old one', () => { + const state = makeThreadState([]) + addMessagesToThreadState(state, [makeAttachmentMessage({fileURL: 'http://127.0.0.1:5000/f'})], {}) + addMessagesToThreadState(state, [makeAttachmentMessage({fileURL: 'http://127.0.0.1:6000/f'})], {}) + expect((state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).fileURL).toBe( + 'http://127.0.0.1:6000/f' + ) + }) + + test.each([ + { + field: 'fileURL', + make: (url: string) => makeAttachmentMessage({fileURL: url}), + read: (state: WritableConversationThreadMessageState) => + (state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).fileURL, + }, + { + field: 'previewURL', + make: (url: string) => makeAttachmentMessage({previewURL: url}), + read: (state: WritableConversationThreadMessageState) => + (state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).previewURL, + }, + ])('$field: empty or garbage keeps the existing value, a real url replaces it', ({make, read}) => { + const empty = makeThreadState([]) + addMessagesToThreadState(empty, [make(validUrl)], {}) + addMessagesToThreadState(empty, [make('')], {}) + expect(read(empty)).toBe(validUrl) + + const garbage = makeThreadState([]) + addMessagesToThreadState(garbage, [make(validUrl)], {}) + addMessagesToThreadState(garbage, [make(garbageUrl)], {}) + expect(read(garbage)).toBe(validUrl) + + const replaced = makeThreadState([]) + addMessagesToThreadState(replaced, [make('http://127.0.0.1:5000/f')], {}) + addMessagesToThreadState(replaced, [make(validUrl)], {}) + expect(read(replaced)).toBe(validUrl) + }) +}) diff --git a/shared/chat/conversation/thread-message-state.tsx b/shared/chat/conversation/thread-message-state.tsx index a2c575bb253b..da18e4485828 100644 --- a/shared/chat/conversation/thread-message-state.tsx +++ b/shared/chat/conversation/thread-message-state.tsx @@ -143,6 +143,13 @@ const maybeGetOrdinalByMessageID = ( ) => getOrdinalForMessageID(state.messageMap, state.pendingOutboxToOrdinal, messageID, state.messageIDToOrdinal) +// The service's local http server can be stopped or mid-restart when an update carrying a +// GetURL-derived value lands: that value can come back as '' or as a base-less URL with only +// query params appended, never as a garbage http:// value. Keep whatever was already rendering +// until a real replacement arrives. +const keepUrl = (next: string | undefined, prev: string | undefined) => + next?.startsWith('http://') ? next : prev ?? next + const mergeMessage = ( existing: WritableDraft, incoming: WritableDraft @@ -170,6 +177,11 @@ const mergeMessage = ( } else { existingRecord[key] = val } + } else if (key === 'fileURL' || key === 'previewURL') { + const next = keepUrl(val as string | undefined, cur as string | undefined) + if (cur !== next) { + existingRecord[key] = next + } } else if (cur !== val) { existingRecord[key] = val } diff --git a/shared/common-adapters/image.tsx b/shared/common-adapters/image.tsx index 26d22748be6b..5256088bd107 100644 --- a/shared/common-adapters/image.tsx +++ b/shared/common-adapters/image.tsx @@ -3,6 +3,7 @@ import * as Styles from '@/styles' import type {ImageLoadEventData, ImageErrorEventData} from 'expo-image' import {Image as ExpoImage} from 'expo-image' import LoadingStateView from './loading-state-view' +import {isLocalhostSrc, retryLocalhostSrc} from './localhost-src' import type {StylesCrossPlatform} from '@/styles' import {useConfigState} from '@/stores/config' import {useShellState} from '@/stores/shell' @@ -52,16 +53,14 @@ const DesktopImage = (p: Props) => { // on background/inactive and restarts it (new token, possibly new port) on foreground, so a // load racing the restart gets connection refused. Those are worth retrying; remote srcs keep // the old fail-once behavior. -const isLocalhostSrc = (src: Props['src']): src is string => - typeof src === 'string' && src.startsWith('http://127.0.0.1:') - const maxRetries = 3 const NativeImage = (p: Props) => { const {showLoadingStateUntilLoaded, src, onLoad, onError, style, contentFit = 'contain', allowDownscaling} = p const [loading, setLoading] = React.useState(!showLoadingStateUntilLoaded) const [lastSrc, setLastSrc] = React.useState(src) - const [attempt, setAttempt] = React.useState(0) + // the retried src is resolved against the http server when the retry is scheduled + const [retry, setRetry] = React.useState<{attempt: number; src?: string}>({attempt: 0}) const retryable = isLocalhostSrc(src) const failedRef = React.useRef(false) const triesRef = React.useRef(0) @@ -76,8 +75,17 @@ const NativeImage = (p: Props) => { if (lastSrc !== src) { setLastSrc(src) setLoading(true) - setAttempt(0) + setRetry({attempt: 0}) + } + + const scheduleRetry = () => { + if (!isLocalhostSrc(src)) return + setRetry(r => ({ + attempt: r.attempt + 1, + src: retryLocalhostSrc(src, r.attempt + 1, useConfigState.getState().httpSrv), + })) } + const scheduleRetryEvent = React.useEffectEvent(scheduleRetry) React.useEffect(() => { triesRef.current = 0 @@ -90,9 +98,7 @@ const NativeImage = (p: Props) => { if (retryable && triesRef.current < maxRetries) { triesRef.current++ clearTimeout(timerRef.current) - timerRef.current = setTimeout(() => { - setAttempt(a => a + 1) - }, 1000 * 2 ** (triesRef.current - 1)) + timerRef.current = setTimeout(scheduleRetry, 1000 * 2 ** (triesRef.current - 1)) return } setLoading(false) @@ -111,7 +117,7 @@ const NativeImage = (p: Props) => { failedRef.current = false triesRef.current = 0 setLoading(true) - setAttempt(a => a + 1) + scheduleRetryEvent() } const unsubConfig = useConfigState.subscribe((s, prev) => { if (s.httpSrv.address !== prev.httpSrv.address || s.httpSrv.token !== prev.httpSrv.token) { @@ -130,9 +136,8 @@ const NativeImage = (p: Props) => { } }, [retryable]) - // cache-buster forces expo-image to actually refetch; recyclingKey stays on the original - // src so the view isn't blanked by retries - const srcToUse = retryable && attempt > 0 ? `${src}${src.includes('?') ? '&' : '?'}kbRetry=${attempt}` : src + // recyclingKey stays on the original src so the view isn't blanked by retries + const srcToUse = retry.src ?? src const recyclingKey = typeof src === 'string' ? src : Array.isArray(src) ? src[0]?.uri : String(src) return ( diff --git a/shared/common-adapters/localhost-src.test.ts b/shared/common-adapters/localhost-src.test.ts new file mode 100644 index 000000000000..c0d324d5d1dc --- /dev/null +++ b/shared/common-adapters/localhost-src.test.ts @@ -0,0 +1,44 @@ +/// +import {isLocalhostSrc, retryLocalhostSrc} from './localhost-src' + +const httpSrv = {address: '127.0.0.1:61234', token: 'newtoken'} + +test('only local service srcs are retryable', () => { + expect(isLocalhostSrc('http://127.0.0.1:5000/av?name=testuser')).toBe(true) + expect(isLocalhostSrc('https://keybase.io/images/testuser.png')).toBe(false) + expect(isLocalhostSrc(3)).toBe(false) +}) + +test('a retry points a baked attachment url at the current server port', () => { + const src = 'http://127.0.0.1:5000/at?key=abc&prev=true&noanim=false&isemoji=false' + expect(retryLocalhostSrc(src, 1, httpSrv)).toBe( + 'http://127.0.0.1:61234/at?key=abc&prev=true&noanim=false&isemoji=false&kbRetry=1' + ) +}) + +test('a retry replaces the token param when there is one', () => { + const src = 'http://127.0.0.1:5000/av?typ=user&name=testuser&token=oldtoken&count=0' + expect(retryLocalhostSrc(src, 2, httpSrv)).toBe( + 'http://127.0.0.1:61234/av?typ=user&name=testuser&token=newtoken&count=0&kbRetry=2' + ) +}) + +test('a service restart on the same port still carries the new token', () => { + const src = 'http://127.0.0.1:61234/av?typ=user&name=testuser&token=oldtoken&count=0' + expect(retryLocalhostSrc(src, 1, {address: '127.0.0.1:61234', token: 'newtoken'})).toBe( + 'http://127.0.0.1:61234/av?typ=user&name=testuser&token=newtoken&count=0&kbRetry=1' + ) +}) + +test('a retry keeps the baked address when the current one is unknown', () => { + const src = 'http://127.0.0.1:5000/at?key=abc' + expect(retryLocalhostSrc(src, 1, {address: '', token: ''})).toBe('http://127.0.0.1:5000/at?key=abc&kbRetry=1') +}) + +test('a kbfs src keeps its own server and token', () => { + // kbfs runs a second local server on its own port with its own token + const src = 'http://127.0.0.1:7000/files/private/testuser/cat.png?token=kbfstoken&viewTypeInvariance=1' + expect(retryLocalhostSrc(src, 1, httpSrv)).toBe( + 'http://127.0.0.1:7000/files/private/testuser/cat.png?token=kbfstoken&viewTypeInvariance=1&kbRetry=1' + ) +}) diff --git a/shared/common-adapters/localhost-src.tsx b/shared/common-adapters/localhost-src.tsx new file mode 100644 index 000000000000..f2420f422777 --- /dev/null +++ b/shared/common-adapters/localhost-src.tsx @@ -0,0 +1,32 @@ +const localhostPrefix = /^http:\/\/127\.0\.0\.1:\d+/ + +// The service's own endpoints: "at" (go/chat/attachment_httpsrv.go), "av" (go/avatars/srv.go) and +// "map" (go/chat/maps/srv.go). KBFS serves /files/ from a second local server with its own port and +// its own token, so a src has to be matched against these before anything is repointed. +const serviceSrc = /^http:\/\/127\.0\.0\.1:\d+\/(?:at|av|map)(?=[?#]|$)/ + +export const isLocalhostSrc = (src: unknown): src is string => + typeof src === 'string' && localhostPrefix.test(src) + +// The service can restart its http server on a new port, but chat bakes the address into +// attachment and emoji URLs, so a retry points the src at wherever the server is now. A service +// process restart draws a fresh port and mints a new per-process token (see +// go/kbhttp/manager/manager.go), and a reconnect alone doesn't refetch already-rendered thread +// data, so the token also needs rewriting or a stale token= keeps failing forever. The +// cache-buster forces expo-image to actually refetch, and is all a non-service src gets. +export const retryLocalhostSrc = ( + src: string, + attempt: number, + httpSrv: {address: string; token: string} +) => { + let next = src + if (serviceSrc.test(src)) { + if (httpSrv.address) { + next = next.replace(localhostPrefix, `http://${httpSrv.address}`) + } + if (httpSrv.token) { + next = next.replace(/([?&]token=)[^&#]*/, `$1${httpSrv.token}`) + } + } + return `${next}${next.includes('?') ? '&' : '?'}kbRetry=${attempt}` +}