[supervisor] Frontend: re-try + track "checkReady" (#20175)

geropl · web-flow · commit 414ee459833e · 2024-09-05T10:54:48.000-04:00
* [ws-proxy] Log whenever we can't connect to an upstream

* [protocol] Fix linter error

* [supervisor] Frontend: re-connect every 5s + track behavior

* [supervisor] Frontend: Guard connection re-try with feature flag "supervisor_check_ready_retry"
diff --git a/components/dashboard/src/service/service.tsx b/components/dashboard/src/service/service.tsx
@@ -184,6 +184,7 @@ export class IDEFrontendService implements IDEFrontendDashboardService.IServer {
         private clientWindow: Window,
     ) {
         this.processServerInfo();
+        this.sendFeatureFlagsUpdate();
         window.addEventListener("message", (event: MessageEvent) => {
             if (IDEFrontendDashboardService.isTrackEventData(event.data)) {
                 this.trackEvent(event.data.msg);
@@ -200,6 +201,9 @@ export class IDEFrontendService implements IDEFrontendDashboardService.IServer {
             if (IDEFrontendDashboardService.isOpenDesktopIDE(event.data)) {
                 this.openDesktopIDE(event.data.url);
             }
+            if (IDEFrontendDashboardService.isFeatureFlagsRequestEventData(event.data)) {
+                this.sendFeatureFlagsUpdate();
+            }
         });
         window.addEventListener("unload", () => {
             if (!this.instanceID) {
@@ -376,6 +380,23 @@ export class IDEFrontendService implements IDEFrontendDashboardService.IServer {
         );
     }
 
+    private async sendFeatureFlagsUpdate() {
+        const supervisor_check_ready_retry = await getExperimentsClient().getValueAsync(
+            "supervisor_check_ready_retry",
+            false,
+            {
+                gitpodHost: gitpodHostUrl.toString(),
+            },
+        );
+        this.clientWindow.postMessage(
+            {
+                type: "ide-feature-flag-update",
+                flags: { supervisor_check_ready_retry },
+            } as IDEFrontendDashboardService.FeatureFlagsUpdateEventData,
+            "*",
+        );
+    }
+
     relocate(url: string): void {
         this.clientWindow.postMessage(
             { type: "ide-relocate", url } as IDEFrontendDashboardService.RelocateEventData,
diff --git a/components/gitpod-protocol/src/frontend-dashboard-service.ts b/components/gitpod-protocol/src/frontend-dashboard-service.ts
@@ -69,6 +69,15 @@ export namespace IDEFrontendDashboardService {
         info: Info;
     }
 
+    export interface FeatureFlagsUpdateEventData {
+        type: "ide-feature-flag-update";
+        flags: { supervisor_check_ready_retry: boolean };
+    }
+
+    export interface FeatureFlagsRequestEventData {
+        type: "ide-feature-flag-request";
+    }
+
     export interface HeartbeatEventData {
         type: "ide-heartbeat";
     }
@@ -101,6 +110,14 @@ export namespace IDEFrontendDashboardService {
         return obj != null && typeof obj === "object" && obj.type === "ide-info-update";
     }
 
+    export function isFeatureFlagsUpdateEventData(obj: any): obj is FeatureFlagsUpdateEventData {
+        return obj != null && typeof obj === "object" && obj.type === "ide-feature-flag-update";
+    }
+
+    export function isFeatureFlagsRequestEventData(obj: any): obj is FeatureFlagsRequestEventData {
+        return obj != null && typeof obj === "object" && obj.type === "ide-feature-flag-request";
+    }
+
     export function isHeartbeatEventData(obj: any): obj is HeartbeatEventData {
         return obj != null && typeof obj === "object" && obj.type === "ide-heartbeat";
     }
diff --git a/components/gitpod-protocol/src/util/timeout.spec.ts b/components/gitpod-protocol/src/util/timeout.spec.ts
@@ -0,0 +1,59 @@
+/**
+ * Copyright (c) 2020 Gitpod GmbH. All rights reserved.
+ * Licensed under the GNU Affero General Public License (AGPL).
+ * See License.AGPL.txt in the project root for license information.
+ */
+
+import * as chai from "chai";
+const expect = chai.expect;
+import { suite, test } from "@testdeck/mocha";
+import { Timeout } from "./timeout";
+
+@suite()
+export class TimeoutSpec {
+    @test
+    async testSimpleRun() {
+        const timeout = new Timeout(1);
+        timeout.start();
+        await timeout.await();
+        expect(timeout.signal()?.aborted).to.be.true;
+    }
+
+    @test
+    async testSimpleRunNotStarted() {
+        const timeout = new Timeout(1);
+        await timeout.await();
+        expect(timeout.signal()).to.be.undefined;
+    }
+
+    @test
+    async testRestart() {
+        const timeout = new Timeout(20);
+        timeout.start();
+        await timeout.await();
+        expect(timeout.signal()?.aborted).to.be.true;
+
+        timeout.restart();
+        expect(timeout.signal()).to.not.be.undefined;
+        expect(timeout.signal()?.aborted).to.be.false;
+        await timeout.await();
+        expect(timeout.signal()?.aborted).to.be.true;
+    }
+
+    @test
+    async testClear() {
+        const timeout = new Timeout(1000);
+        timeout.restart();
+        timeout.clear();
+        expect(timeout.signal()).to.be.undefined;
+    }
+
+    @test
+    async testAbortCondition() {
+        const timeout = new Timeout(1, () => false); // will never trigger abort
+        timeout.start();
+        await new Promise((resolve) => setTimeout(resolve, 50));
+        expect(timeout.signal()).to.not.be.undefined;
+        expect(timeout.signal()?.aborted).to.be.false;
+    }
+}
diff --git a/components/gitpod-protocol/src/util/timeout.ts b/components/gitpod-protocol/src/util/timeout.ts
@@ -0,0 +1,80 @@
+/**
+ * Copyright (c) 2024 Gitpod GmbH. All rights reserved.
+ * Licensed under the GNU Affero General Public License (AGPL).
+ * See License.AGPL.txt in the project root for license information.
+ */
+
+/**
+ * A restartable timeout, based on an AbortController + setTimeout.
+ *
+ * Note: When cleared/reset, the AbortController is _NOT_ aborted.
+ */
+export class Timeout {
+    private _timer: NodeJS.Timeout | undefined;
+    private _abortController: AbortController | undefined;
+
+    /**
+     * @param timeout The timeout in milliseconds.
+     * @param abortCondition An optional condition evaluated on timeout: If set, the abort is only emitted if it evaluates to true.
+     */
+    constructor(readonly timeout: number, readonly abortCondition?: () => boolean) {}
+
+    /**
+     * Starts a new timeout (and clears the old one). Identical to `reset`.
+     */
+    public start() {
+        this.restart();
+    }
+
+    /**
+     * Starts a new timeout (and clears the old one).
+     */
+    public restart() {
+        this.clear();
+
+        const abortController = new AbortController();
+        this._abortController = abortController;
+        this._timer = setTimeout(() => {
+            if (this.abortCondition && !this.abortCondition()) {
+                return;
+            }
+
+            abortController.abort();
+        }, this.timeout);
+    }
+
+    /**
+     * Clears the current timeout.
+     */
+    public clear() {
+        if (this._timer) {
+            clearTimeout(this._timer);
+            this._timer = undefined;
+        }
+        if (this._abortController) {
+            this._abortController = undefined;
+        }
+    }
+
+    /**
+     * Convenience method to await the timeout.
+     * @returns
+     */
+    public async await(): Promise<boolean> {
+        const abortController = this._abortController;
+        if (!abortController) {
+            return false;
+        }
+
+        return new Promise((resolve) => {
+            abortController.signal.addEventListener("abort", () => resolve(true));
+        });
+    }
+
+    /**
+     * @returns The AbortSignal of the current timeout, if one is active.
+     */
+    public signal(): AbortSignal | undefined {
+        return this._abortController?.signal;
+    }
+}
diff --git a/components/gitpod-protocol/tsconfig.json b/components/gitpod-protocol/tsconfig.json
@@ -16,8 +16,7 @@
             "es6",
             "es2018",
             "dom",
-            "ES2018.Regexp",
-            "DOM.AsyncIterable"
+            "ES2018.Regexp"
         ],
         "sourceMap": true,
         "declaration": true,
diff --git a/components/supervisor/frontend/src/ide/supervisor-service-client.ts b/components/supervisor/frontend/src/ide/supervisor-service-client.ts
@@ -11,23 +11,17 @@ import {
 } from "@gitpod/supervisor-api-grpc/lib/status_pb";
 import { WorkspaceInfoResponse } from "@gitpod/supervisor-api-grpc/lib/info_pb";
 import { workspaceUrl } from "../shared/urls";
+import { FrontendDashboardServiceClient } from "../shared/frontend-dashboard-service";
+import { Timeout } from "@gitpod/gitpod-protocol/lib/util/timeout";
 
 export class SupervisorServiceClient {
-    private static _instance: SupervisorServiceClient | undefined;
-    static get(): SupervisorServiceClient {
-        if (!SupervisorServiceClient._instance) {
-            SupervisorServiceClient._instance = new SupervisorServiceClient();
-        }
-        return SupervisorServiceClient._instance;
-    }
-
     readonly supervisorReady = this.checkReady("supervisor");
     readonly ideReady = this.supervisorReady.then(() => this.checkReady("ide"));
     readonly contentReady = Promise.all([this.supervisorReady]).then(() => this.checkReady("content"));
     readonly getWorkspaceInfoPromise = this.supervisorReady.then(() => this.getWorkspaceInfo());
     private _supervisorWillShutdown: Promise<void> | undefined;
 
-    private constructor() {}
+    constructor(readonly serviceClient: FrontendDashboardServiceClient) {}
 
     public get supervisorWillShutdown() {
         if (!this._supervisorWillShutdown) {
@@ -84,13 +78,44 @@ export class SupervisorServiceClient {
         if (kind == "supervisor") {
             wait = "";
         }
+
+        // track whenever a) we are done, or b) we try to connect (again)
+        const trackCheckReady = (p: { aborted?: boolean }, err?: any): void => {
+            const props: Record<string, string> = {
+                component: "supervisor-frontend",
+                instanceId: this.serviceClient.latestInfo?.instanceId ?? "",
+                userId: this.serviceClient.latestInfo?.loggedUserId ?? "",
+                readyKind: kind,
+            };
+            if (err) {
+                props.errorName = err.name;
+                props.errorStack = err.message ?? String(err);
+            }
+
+            props.aborted = String(!!p.aborted);
+            props.wait = wait;
+
+            this.serviceClient.trackEvent({
+                event: "supervisor_check_ready",
+                properties: props,
+            });
+        };
+
+        // setup a timeout, which is meant to re-establish the connection every 5 seconds
+        let isError = false;
+        const timeout = new Timeout(5000, () => this.serviceClient.isCheckReadyRetryEnabled());
         try {
+            timeout.restart();
+
             const wsSupervisorStatusUrl = workspaceUrl.with(() => {
                 return {
                     pathname: "/_supervisor/v1/status/" + kind + wait,
                 };
             });
-            const response = await fetch(wsSupervisorStatusUrl.toString(), { credentials: "include" });
+            const response = await fetch(wsSupervisorStatusUrl.toString(), {
+                credentials: "include",
+                signal: timeout.signal(),
+            });
             let result;
             if (response.ok) {
                 result = await response.json();
@@ -112,6 +137,16 @@ export class SupervisorServiceClient {
             );
         } catch (e) {
             console.debug(`failed to check whether ${kind} is ready, trying again...`, e);
+
+            // we want to track this kind of errors, as they are on the critical path (of revealing the workspace)
+            isError = true;
+            trackCheckReady({ aborted: timeout.signal()?.aborted }, e);
+        } finally {
+            if (!isError) {
+                // make sure we don't track twice in case of an error
+                trackCheckReady({ aborted: timeout.signal()?.aborted });
+            }
+            timeout.clear();
         }
         return this.checkReady(kind, true);
     }
diff --git a/components/supervisor/frontend/src/index.ts b/components/supervisor/frontend/src/index.ts
@@ -85,9 +85,9 @@ LoadingFrame.load().then(async (loading) => {
     window.gitpod.encrypt = frontendDashboardServiceClient.encrypt.bind(frontendDashboardServiceClient);
     window.gitpod.isEncryptedData = frontendDashboardServiceClient.isEncryptedData.bind(frontendDashboardServiceClient);
 
-    (async () => {
-        const supervisorServiceClient = SupervisorServiceClient.get();
+    const supervisorServiceClient = new SupervisorServiceClient(frontendDashboardServiceClient);
 
+    (async () => {
         let hideDesktopIde = false;
         const hideDesktopIdeEventListener = frontendDashboardServiceClient.onOpenBrowserIDE(() => {
             hideDesktopIdeEventListener.dispose();
@@ -271,7 +271,6 @@ LoadingFrame.load().then(async (loading) => {
                 });
             });
         }
-        const supervisorServiceClient = SupervisorServiceClient.get();
         if (debugWorkspace) {
             supervisorServiceClient.supervisorWillShutdown.then(() => {
                 window.open("", "_self")?.close();
diff --git a/components/supervisor/frontend/src/shared/frontend-dashboard-service.ts b/components/supervisor/frontend/src/shared/frontend-dashboard-service.ts
@@ -26,6 +26,7 @@ export class FrontendDashboardServiceClient implements IDEFrontendDashboardServi
 
     private resolveInit!: () => void;
     private initPromise = new Promise<void>((resolve) => (this.resolveInit = resolve));
+    private featureFlags: Partial<IDEFrontendDashboardService.FeatureFlagsUpdateEventData["flags"]> = {};
 
     private version?: number;
 
@@ -59,7 +60,11 @@ export class FrontendDashboardServiceClient implements IDEFrontendDashboardServi
             if (IDEFrontendDashboardService.isOpenBrowserIDE(event.data)) {
                 this.onOpenBrowserIDEEmitter.fire(undefined);
             }
+            if (IDEFrontendDashboardService.isFeatureFlagsUpdateEventData(event.data)) {
+                this.featureFlags = event.data.flags;
+            }
         });
+        this.requestFreshFeatureFlags();
     }
     initialize(): Promise<void> {
         return this.initPromise;
@@ -140,6 +145,17 @@ export class FrontendDashboardServiceClient implements IDEFrontendDashboardServi
             serverUrl.url.origin,
         );
     }
+
+    requestFreshFeatureFlags(): void {
+        window.postMessage(
+            { type: "ide-feature-flag-request" } as IDEFrontendDashboardService.FeatureFlagsRequestEventData,
+            serverUrl.url.origin,
+        );
+    }
+
+    isCheckReadyRetryEnabled(): boolean {
+        return !!this.featureFlags.supervisor_check_ready_retry;
+    }
 }
 
 function isSerializedEncryptedData(obj: any): obj is { iv: string; encrypted: string; tag: string } {
diff --git a/components/ws-proxy/pkg/proxy/pass.go b/components/ws-proxy/pkg/proxy/pass.go
@@ -210,6 +210,12 @@ func withHTTPErrorHandler(h http.Handler) proxyPassOpt {
 	}
 }
 
+func withErrorHandler(h errorHandler) proxyPassOpt {
+	return func(cfg *proxyPassConfig) {
+		cfg.ErrorHandler = h
+	}
+}
+
 func createDefaultTransport(config *TransportConfig) *http.Transport {
 	// TODO equivalent of client_max_body_size 2048m; necessary ???
 	// this is based on http.DefaultTransport, with some values exposed to config
diff --git a/components/ws-proxy/pkg/proxy/routes.go b/components/ws-proxy/pkg/proxy/routes.go

Original file line number	Diff line number	Diff line change
`@@ -210,6 +210,12 @@ func withHTTPErrorHandler(h http.Handler) proxyPassOpt {`
`210`	`210`	`}`
`211`	`211`	`}`
`212`	`212`
	`213`	`+func withErrorHandler(h errorHandler) proxyPassOpt {`
	`214`	`+ return func(cfg *proxyPassConfig) {`
	`215`	`+ cfg.ErrorHandler = h`
	`216`	`+ }`
	`217`	`+}`
	`218`	`+`
`213`	`219`	`func createDefaultTransport(config TransportConfig) http.Transport {`
`214`	`220`	`// TODO equivalent of client_max_body_size 2048m; necessary ???`
`215`	`221`	`// this is based on http.DefaultTransport, with some values exposed to config`