diff --git a/e2e/playwright/test-network-and-connection-issues.spec.ts b/e2e/playwright/test-network-and-connection-issues.spec.ts index 40fde6537..157607817 100644 --- a/e2e/playwright/test-network-and-connection-issues.spec.ts +++ b/e2e/playwright/test-network-and-connection-issues.spec.ts @@ -61,7 +61,7 @@ test.describe('Test network related behaviors', () => { }) // Expect the network to be down - await expect(networkToggle).toContainText('Problem') + await expect(networkToggle).toContainText('Network health (Offline)') // Click the network widget await networkWidget.click() @@ -156,7 +156,8 @@ test.describe('Test network related behaviors', () => { // Expect the network to be down await networkToggle.hover() - await expect(networkToggle).toContainText('Problem') + + await expect(networkToggle).toContainText('Network health (Offline)') // Ensure we are not in sketch mode await expect( diff --git a/e2e/playwright/test-utils.ts b/e2e/playwright/test-utils.ts index aa1e9ca08..294367bc5 100644 --- a/e2e/playwright/test-utils.ts +++ b/e2e/playwright/test-utils.ts @@ -364,11 +364,6 @@ export async function getUtils(page: Page, test_?: typeof test) { ) } - // Chrome devtools protocol session only works in Chromium - const browserType = page.context().browser()?.browserType().name() - const cdpSession = - browserType !== 'chromium' ? null : await page.context().newCDPSession(page) - const util = { waitForAuthSkipAppStart: () => waitForAuthAndLsp(page), waitForPageLoad: () => waitForPageLoad(page), @@ -489,15 +484,9 @@ export async function getUtils(page: Page, test_?: typeof test) { emulateNetworkConditions: async ( networkOptions: Protocol.Network.emulateNetworkConditionsParameters ) => { - if (cdpSession === null) { - // Use a fail safe if we can't simulate disconnect (on Safari) - return page.evaluate('window.engineCommandManager.tearDown()') - } - - return cdpSession?.send( - 'Network.emulateNetworkConditions', - networkOptions - ) + return networkOptions.offline + ? page.evaluate('window.engineCommandManager.offline()') + : page.evaluate('window.engineCommandManager.online()') }, toNormalizedCode(text: string) { diff --git a/src/App.tsx b/src/App.tsx index e853746d0..710cd213a 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -114,6 +114,9 @@ export function App() { // by the Projects view. billingActor.send({ type: BillingTransition.Update, apiToken: authToken }) + // Tell engineStream to wait for dependencies to start streaming. + engineStreamActor.send({ type: EngineStreamTransition.WaitForDependencies }) + // When leaving the modeling scene, cut the engine stream. return () => { // When leaving the modeling scene, cut the engine stream. diff --git a/src/clientSideScene/sceneEntities.ts b/src/clientSideScene/sceneEntities.ts index c288761b3..c13c834aa 100644 --- a/src/clientSideScene/sceneEntities.ts +++ b/src/clientSideScene/sceneEntities.ts @@ -3584,7 +3584,8 @@ export class SceneEntities { }) if (!resp) { - return Promise.reject('no response') + console.warn('No response') + return {} as Models['GetSketchModePlane_type'] } if (isArray(resp)) { diff --git a/src/components/EngineStream.tsx b/src/components/EngineStream.tsx index 3b6a1154a..d7be1b700 100644 --- a/src/components/EngineStream.tsx +++ b/src/components/EngineStream.tsx @@ -1,4 +1,3 @@ -import { isPlaywright } from '@src/lib/isPlaywright' import { useAppState } from '@src/AppState' import { ClientSideScene } from '@src/clientSideScene/ClientSideSceneComp' import { ViewControlContextMenu } from '@src/components/ViewControlMenu' @@ -52,8 +51,10 @@ export const EngineStream = (props: { const last = useRef(Date.now()) const [firstPlay, setFirstPlay] = useState(true) - const [isRestartRequestStarting, setIsRestartRequestStarting] = - useState(false) + const [goRestart, setGoRestart] = useState(false) + const [timeoutId, setTimeoutId] = useState< + ReturnType | undefined + >(undefined) const [attemptTimes, setAttemptTimes] = useState<[number, number]>([ 0, TIME_1_SECOND, @@ -85,18 +86,21 @@ export const EngineStream = (props: { const streamIdleMode = settings.app.streamIdleMode.current useEffect(() => { + // Will cause a useEffect loop if not checked for. + if (engineStreamState.context.videoRef.current !== null) return engineStreamActor.send({ type: EngineStreamTransition.SetVideoRef, videoRef: { current: videoRef.current }, }) - }, [videoRef.current]) + }, [videoRef.current, engineStreamState]) useEffect(() => { + if (engineStreamState.context.canvasRef.current !== null) return engineStreamActor.send({ type: EngineStreamTransition.SetCanvasRef, canvasRef: { current: canvasRef.current }, }) - }, [canvasRef.current]) + }, [canvasRef.current, engineStreamState]) useEffect(() => { engineStreamActor.send({ @@ -131,24 +135,6 @@ export const EngineStream = (props: { }) } - useEffect(() => { - // Only try to start the stream if we're stopped or think we're done - // waiting for dependencies. - if ( - !( - engineStreamState.value === EngineStreamState.WaitingForDependencies || - engineStreamState.value === EngineStreamState.Stopped - ) - ) - return - - // Don't bother trying to connect if the auth token is empty. - // We have the checks in the machine but this can cause a hot loop. - if (!engineStreamState.context.authToken) return - - startOrReconfigureEngine() - }, [engineStreamState, setAppState]) - // I would inline this but it needs to be a function for removeEventListener. const play = () => { engineStreamActor.send({ @@ -174,12 +160,13 @@ export const EngineStream = (props: { console.log('scene is ready, execute kcl') const kmp = kclManager.executeCode().catch(trap) - if (!firstPlay) return - - setFirstPlay(false) // Reset the restart timeouts setAttemptTimes([0, TIME_1_SECOND]) + console.log(firstPlay) + if (!firstPlay) return + + setFirstPlay(false) console.log('firstPlay true, zoom to fit') kmp .then(async () => { @@ -211,51 +198,76 @@ export const EngineStream = (props: { // We do a back-off restart, using a fibonacci sequence, since it // has a nice retry time curve (somewhat quick then exponential) const attemptRestartIfNecessary = () => { - if (isRestartRequestStarting) return - setIsRestartRequestStarting(true) - setTimeout(() => { - engineStreamState.context.videoRef.current?.pause() - engineCommandManager.tearDown() - startOrReconfigureEngine() - setFirstPlay(false) - setIsRestartRequestStarting(false) - }, attemptTimes[0] + attemptTimes[1]) + // Timeout already set. + if (timeoutId) return + + setTimeoutId( + setTimeout(() => { + engineStreamState.context.videoRef.current?.pause() + engineCommandManager.tearDown() + startOrReconfigureEngine() + setFirstPlay(true) + + setTimeoutId(undefined) + setGoRestart(false) + }, attemptTimes[0] + attemptTimes[1]) + ) setAttemptTimes([attemptTimes[1], attemptTimes[0] + attemptTimes[1]]) } - // Poll that we're connected. If not, send a reset signal. - // Do not restart if we're in idle mode. - const connectionCheckIntervalId = setInterval(() => { - // SKIP DURING TESTS BECAUSE IT WILL MESS WITH REUSING THE - // ELECTRON INSTANCE. - if (isPlaywright()) { + const onOffline = () => { + if ( + !( + EngineConnectionStateType.Disconnected === + engineCommandManager.engineConnection?.state.type || + EngineConnectionStateType.Disconnecting === + engineCommandManager.engineConnection?.state.type + ) + ) { return } - - // Don't try try to restart if we're already connected! - const hasEngineConnectionInst = engineCommandManager.engineConnection - const isDisconnected = - engineCommandManager.engineConnection?.state.type === - EngineConnectionStateType.Disconnected - const inIdleMode = engineStreamState.value === EngineStreamState.Paused - if ((hasEngineConnectionInst && !isDisconnected) || inIdleMode) return - + engineStreamActor.send({ type: EngineStreamTransition.Stop }) attemptRestartIfNecessary() - }, TIME_1_SECOND) + } + + if ( + !goRestart && + engineStreamState.value === EngineStreamState.WaitingForDependencies + ) { + setGoRestart(true) + } + + if (goRestart && !timeoutId) { + attemptRestartIfNecessary() + } engineCommandManager.addEventListener( EngineCommandManagerEvents.EngineRestartRequest, attemptRestartIfNecessary ) - return () => { - clearInterval(connectionCheckIntervalId) + engineCommandManager.addEventListener( + EngineCommandManagerEvents.Offline, + onOffline + ) + + return () => { engineCommandManager.removeEventListener( EngineCommandManagerEvents.EngineRestartRequest, attemptRestartIfNecessary ) + engineCommandManager.removeEventListener( + EngineCommandManagerEvents.Offline, + onOffline + ) } - }, [engineStreamState, attemptTimes, isRestartRequestStarting]) + }, [ + engineStreamState, + attemptTimes, + goRestart, + timeoutId, + engineCommandManager.engineConnection?.state.type, + ]) useEffect(() => { // If engineStreamMachine is already reconfiguring, bail. @@ -269,7 +281,7 @@ export const EngineStream = (props: { const canvas = engineStreamState.context.canvasRef?.current if (!canvas) return - new ResizeObserver(() => { + const observer = new ResizeObserver(() => { // Prevents: // `Uncaught ResizeObserver loop completed with undelivered notifications` window.requestAnimationFrame(() => { @@ -280,13 +292,19 @@ export const EngineStream = (props: { if ( (Math.abs(video.width - window.innerWidth) > 4 || Math.abs(video.height - window.innerHeight) > 4) && - !engineStreamState.matches(EngineStreamState.WaitingToPlay) + engineStreamState.matches(EngineStreamState.Playing) ) { timeoutStart.current = Date.now() startOrReconfigureEngine() } }) - }).observe(document.body) + }) + + observer.observe(document.body) + + return () => { + observer.disconnect() + } }, [engineStreamState.value]) /** @@ -345,8 +363,21 @@ export const EngineStream = (props: { timeoutStart.current = null } else if (timeoutStart.current) { const elapsed = Date.now() - timeoutStart.current - if (elapsed >= IDLE_TIME_MS) { + // Don't pause if we're already disconnected. + if ( + // It's unnecessary to once again setup an event listener for + // offline/online to capture this state, when this state already + // exists on the window.navigator object. In hindsight it makes + // me (lee) regret we set React state variables such as + // isInternetConnected in other files when we could check this + // object instead. + engineCommandManager.engineConnection?.state.type === + EngineConnectionStateType.ConnectionEstablished && + elapsed >= IDLE_TIME_MS && + engineStreamState.value === EngineStreamState.Playing + ) { timeoutStart.current = null + console.log('PAUSING') engineStreamActor.send({ type: EngineStreamTransition.Pause }) } } @@ -357,7 +388,7 @@ export const EngineStream = (props: { return () => { window.cancelAnimationFrame(frameId) } - }, [modelingMachineState]) + }, [modelingMachineState, engineStreamState.value]) useEffect(() => { if (!streamIdleMode) return @@ -370,9 +401,18 @@ export const EngineStream = (props: { return } - if (engineStreamState.value === EngineStreamState.Paused) { - startOrReconfigureEngine() - } + engineStreamActor.send({ + type: EngineStreamTransition.Resume, + modelingMachineActorSend, + settings: settingsEngine, + setAppState, + onMediaStream(mediaStream: MediaStream) { + engineStreamActor.send({ + type: EngineStreamTransition.SetMediaStream, + mediaStream, + }) + }, + }) timeoutStart.current = Date.now() } @@ -471,7 +511,11 @@ export const EngineStream = (props: { } sendSelectEventToEngine(e) - .then(({ entity_id }) => { + .then((result) => { + if (!result) { + return + } + const { entity_id } = result if (!entity_id) { // No entity selected. This is benign return diff --git a/src/hooks/useNetworkStatus.tsx b/src/hooks/useNetworkStatus.tsx index 581d3276e..a6e8219e7 100644 --- a/src/hooks/useNetworkStatus.tsx +++ b/src/hooks/useNetworkStatus.tsx @@ -78,18 +78,19 @@ export function useNetworkStatus() { }, [hasIssues, internetConnected, ping]) useEffect(() => { - const onlineCallback = () => { - setInternetConnected(true) - } const offlineCallback = () => { setInternetConnected(false) setSteps(structuredClone(initialConnectingTypeGroupState)) } - window.addEventListener('online', onlineCallback) - window.addEventListener('offline', offlineCallback) + engineCommandManager.addEventListener( + EngineCommandManagerEvents.Offline, + offlineCallback + ) return () => { - window.removeEventListener('online', onlineCallback) - window.removeEventListener('offline', offlineCallback) + engineCommandManager.removeEventListener( + EngineCommandManagerEvents.Offline, + offlineCallback + ) } }, []) @@ -139,6 +140,8 @@ export function useNetworkStatus() { if ( engineConnectionState.type === EngineConnectionStateType.Connecting ) { + setInternetConnected(true) + const groups = Object.values(nextSteps) for (let group of groups) { for (let step of group) { @@ -168,6 +171,10 @@ export function useNetworkStatus() { if (engineConnectionState.value.type === DisconnectingType.Error) { setError(engineConnectionState.value.value) + } else if ( + engineConnectionState.value.type === DisconnectingType.Quit + ) { + return structuredClone(initialConnectingTypeGroupState) } } } diff --git a/src/lang/std/engineConnection.ts b/src/lang/std/engineConnection.ts index df07c02aa..0b9b2e05a 100644 --- a/src/lang/std/engineConnection.ts +++ b/src/lang/std/engineConnection.ts @@ -1,4 +1,3 @@ -import { TEST } from '@src/env' import type { Models } from '@kittycad/lib' import { VITE_KC_API_WS_MODELING_URL, VITE_KC_DEV_TOKEN } from '@src/env' import { jsAppSettings } from '@src/lib/settings/settingsUtils' @@ -83,6 +82,9 @@ export enum ConnectionError { TooManyConnections, Outage, + // Observed to happen on a local network outage. + PeerConnectionRemoteDisconnected, + // An unknown error is the most severe because it has not been classified // or encountered before. Unknown, @@ -107,6 +109,8 @@ export const CONNECTION_ERROR_TEXT: Record = { 'There are too many open engine connections associated with your account.', [ConnectionError.Outage]: 'We seem to be experiencing an outage. Please visit [status.zoo.dev](https://status.zoo.dev) for updates.', + [ConnectionError.PeerConnectionRemoteDisconnected]: + 'The remote end has disconnected.', [ConnectionError.Unknown]: 'An unexpected error occurred. Please report this to us.', } @@ -226,6 +230,9 @@ export enum EngineConnectionEvents { Opened = 'opened', // (engineConnection: EngineConnection) => void Closed = 'closed', // (engineConnection: EngineConnection) => void NewTrack = 'new-track', // (track: NewTrackArgs) => void + + // A general offline state. + Offline = 'offline', } function toRTCSessionDescriptionInit( @@ -674,9 +681,20 @@ class EngineConnection extends EventTarget { // The remote end broke up with us! :( case 'disconnected': + this.state = { + type: EngineConnectionStateType.Disconnecting, + value: { + type: DisconnectingType.Error, + value: { + error: ConnectionError.PeerConnectionRemoteDisconnected, + context: event, + }, + }, + } this.dispatchEvent( - new CustomEvent(EngineConnectionEvents.RestartRequest, {}) + new CustomEvent(EngineConnectionEvents.Offline, {}) ) + this.disconnectAll() break case 'closed': this.pc?.removeEventListener('icecandidate', this.onIceCandidate) @@ -847,7 +865,6 @@ class EngineConnection extends EventTarget { 'message', this.onDataChannelMessage ) - this.disconnectAll() } this.unreliableDataChannel?.addEventListener( @@ -866,7 +883,6 @@ class EngineConnection extends EventTarget { }, }, } - this.disconnectAll() } this.unreliableDataChannel?.addEventListener( 'error', @@ -956,6 +972,9 @@ class EngineConnection extends EventTarget { this.onNetworkStatusReady ) + this.dispatchEvent( + new CustomEvent(EngineConnectionEvents.Offline, {}) + ) this.disconnectAll() } this.websocket.addEventListener('close', this.onWebSocketClose) @@ -974,8 +993,6 @@ class EngineConnection extends EventTarget { }, } } - - this.disconnectAll() } this.websocket.addEventListener('error', this.onWebSocketError) @@ -1331,6 +1348,9 @@ export enum EngineCommandManagerEvents { // the whole scene is ready (settings loaded) SceneReady = 'scene-ready', + + // we're offline + Offline = 'offline', } /** @@ -1380,6 +1400,7 @@ export class EngineCommandManager extends EventTarget { * This is compared to the {@link outSequence} number to determine if we should ignore * any out-of-order late responses in the unreliable channel. */ + keepForcefulOffline = false inSequence = 1 engineConnection?: EngineConnection commandLogs: CommandLog[] = [] @@ -1453,13 +1474,8 @@ export class EngineCommandManager extends EventTarget { ) } - private onOffline = () => { - console.log('Browser reported network is offline') - if (TEST) { - console.warn('DURING TESTS ENGINECONNECTION.ONOFFLINE WILL DO NOTHING.') - return - } - this.onEngineConnectionRestartRequest() + private onEngineOffline = () => { + this.dispatchEvent(new CustomEvent(EngineCommandManagerEvents.Offline, {})) } idleMode: boolean = false @@ -1494,6 +1510,11 @@ export class EngineCommandManager extends EventTarget { if (settings) { this.settings = settings } + + if (this.keepForcefulOffline) { + return + } + if (width === 0 || height === 0) { return } @@ -1509,8 +1530,6 @@ export class EngineCommandManager extends EventTarget { return } - window.addEventListener('offline', this.onOffline) - let additionalSettings = this.settings.enableSSAO ? '&post_effect=ssao' : '' additionalSettings += '&show_grid=' + (this.settings.showScaleGrid ? 'true' : 'false') @@ -1537,6 +1556,11 @@ export class EngineCommandManager extends EventTarget { this.onEngineConnectionRestartRequest as EventListener ) + this.engineConnection.addEventListener( + EngineConnectionEvents.Offline, + this.onEngineOffline as EventListener + ) + // eslint-disable-next-line @typescript-eslint/no-misused-promises this.onEngineConnectionOpened = async () => { console.log('onEngineConnectionOpened') @@ -1552,9 +1576,9 @@ export class EngineCommandManager extends EventTarget { // Let's restart. console.warn("shit's gone south") console.warn(e) - this.engineConnection?.dispatchEvent( - new CustomEvent(EngineConnectionEvents.RestartRequest, {}) - ) + // this.engineConnection?.dispatchEvent( + // new CustomEvent(EngineConnectionEvents.RestartRequest, {}) + // ) return } @@ -1597,23 +1621,7 @@ export class EngineCommandManager extends EventTarget { console.log('camControlsCameraChange') this._camControlsCameraChange() - // We should eventually only have 1 restoral call. - if (this.idleMode) { - await this.sceneInfra?.camControls.restoreRemoteCameraStateAndTriggerSync() - } else { - // NOTE: This code is old. It uses the old hack to restore camera. - console.log('call default_camera_get_settings') - // eslint-disable-next-line @typescript-eslint/no-floating-promises - await this.sendSceneCommand({ - // CameraControls subscribes to default_camera_get_settings response events - // firing this at connection ensure the camera's are synced initially - type: 'modeling_cmd_req', - cmd_id: uuidv4(), - cmd: { - type: 'default_camera_get_settings', - }, - }) - } + await this.sceneInfra?.camControls.restoreRemoteCameraStateAndTriggerSync() setIsStreamReady(true) @@ -1877,8 +1885,6 @@ export class EngineCommandManager extends EventTarget { tearDown(opts?: { idleMode: boolean }) { this.idleMode = opts?.idleMode ?? false - window.removeEventListener('offline', this.onOffline) - if (this.engineConnection) { for (const [cmdId, pending] of Object.entries(this.pendingCommands)) { pending.reject([ @@ -1928,7 +1934,26 @@ export class EngineCommandManager extends EventTarget { this.engineCommandManager.engineConnection = null } this.engineConnection = undefined + + // It is possible all connections never even started, but we still want + // to signal to the whole application we are "offline". + this.dispatchEvent(new CustomEvent(EngineCommandManagerEvents.Offline, {})) } + + offline() { + this.keepForcefulOffline = true + this.tearDown() + console.log('offline') + } + + online() { + this.keepForcefulOffline = false + this.dispatchEvent( + new CustomEvent(EngineCommandManagerEvents.EngineRestartRequest, {}) + ) + console.log('online') + } + async startNewSession() { this.responseMap = {} } diff --git a/src/lib/selections.ts b/src/lib/selections.ts index 4d94c5569..c9a17b27c 100644 --- a/src/lib/selections.ts +++ b/src/lib/selections.ts @@ -703,7 +703,8 @@ export async function sendSelectEventToEngine( cmd_id: uuidv4(), }) if (!res) { - return Promise.reject('no response') + console.warn('No response') + return undefined } if (isArray(res)) { diff --git a/src/machines/engineStreamMachine.ts b/src/machines/engineStreamMachine.ts index 013887c4d..d23564db6 100644 --- a/src/machines/engineStreamMachine.ts +++ b/src/machines/engineStreamMachine.ts @@ -70,7 +70,6 @@ export async function holdOntoVideoFrameInCanvas( video: HTMLVideoElement, canvas: HTMLCanvasElement ) { - video.pause() canvas.width = video.videoWidth canvas.height = video.videoHeight canvas.style.width = video.videoWidth + 'px' @@ -220,11 +219,14 @@ export const engineStreamMachine = setup({ if (context.videoRef.current && context.canvasRef.current) { await context.videoRef.current.pause() - await holdOntoVideoFrameInCanvas( - context.videoRef.current, - context.canvasRef.current - ) - context.videoRef.current.style.display = 'none' + // It's possible we've already frozen the frame due to a disconnect. + if (context.videoRef.current.style.display !== 'none') { + await holdOntoVideoFrameInCanvas( + context.videoRef.current, + context.canvasRef.current + ) + context.videoRef.current.style.display = 'none' + } } await rootContext.sceneInfra.camControls.saveRemoteCameraState() @@ -365,9 +367,12 @@ export const engineStreamMachine = setup({ }), }, on: { - [EngineStreamTransition.StartOrReconfigureEngine]: { + [EngineStreamTransition.Resume]: { target: EngineStreamState.Resuming, }, + [EngineStreamTransition.Stop]: { + target: EngineStreamState.Stopped, + }, }, }, [EngineStreamState.Stopped]: { @@ -398,12 +403,23 @@ export const engineStreamMachine = setup({ rootContext: args.self.system.get('root').getSnapshot().context, event: args.event, }), + // Usually only fails if there was a disconnection mid-way. + onError: [ + { + target: EngineStreamState.WaitingForDependencies, + reenter: true, + }, + ], }, on: { // The stream can be paused as it's resuming. [EngineStreamTransition.Pause]: { target: EngineStreamState.Paused, }, + // The stream can be stopped as it's resuming. + [EngineStreamTransition.Stop]: { + target: EngineStreamState.Stopped, + }, [EngineStreamTransition.SetMediaStream]: { target: EngineStreamState.Playing, actions: [