Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 58 additions & 14 deletions apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { primeRelayAffinity } from "@superset/workspace-client";
import {
primeRelayAffinity,
type RelayAffinityProbe,
} from "@superset/workspace-client";
import type { Terminal as XTerm } from "@xterm/xterm";
import { posthog } from "renderer/lib/posthog";
import { classifyTerminalFailure } from "./terminalConnectionDiagnostics";
import { createWriteCoalescer, type WriteCoalescer } from "./write-coalescer";

export type ConnectionState = "disconnected" | "connecting" | "open" | "closed";
Expand Down Expand Up @@ -71,6 +76,9 @@ export interface TerminalTransport {
* a parse/render cycle each and overwhelm the renderer (#2241, #2244).
*/
_writeCoalescer: WriteCoalescer | null;
/** Internal: most recent relay `_whoowns` preflight result, used to explain
* a failed connection (see `classifyTerminalFailure`). */
_lastProbe: RelayAffinityProbe | null;
}

const MAX_LOG_ENTRIES = 200;
Expand Down Expand Up @@ -171,6 +179,7 @@ export function createTransport(): TerminalTransport {
_lastLivenessTick: 0,
_resumeListener: null,
_writeCoalescer: null,
_lastProbe: null,
};
}

Expand Down Expand Up @@ -308,6 +317,18 @@ function formatWsEndpoint(wsUrl: string | null): string {
}
}

// Relay-routed terminals live under `/hosts/<id>/...`; local/same-machine
// terminals don't. Only the former go through the relay preflight and can be
// classified with a probe status.
function isRelayHostUrl(wsUrl: string | null): boolean {
if (!wsUrl) return false;
try {
return new URL(wsUrl).pathname.startsWith("/hosts/");
} catch {
return false;
}
}

function formatCloseDetails(event: CloseEvent): string {
const code = event.code || "unknown";
const reason = event.reason ? `, reason: ${event.reason}` : "";
Expand Down Expand Up @@ -395,14 +416,13 @@ export function connect(
// upgrade itself (browser sees 200 → 1006 close), but is on plain HTTP,
// so a quick GET avoids the connect → 1006 → reconnect flicker. Skip
// for non-/hosts URLs (tests, local dev) so connect stays synchronous.
let needsPreFlight = false;
try {
needsPreFlight = new URL(actualUrl).pathname.startsWith("/hosts/");
} catch {
needsPreFlight = false;
}
if (needsPreFlight) {
void primeRelayAffinity(actualUrl).then(openSocket);
// The probe result is also kept to explain a later failure (see close handler).
if (isRelayHostUrl(actualUrl)) {
transport._lastProbe = null;
void primeRelayAffinity(actualUrl).then((probe) => {
transport._lastProbe = probe;
openSocket();
Comment on lines +417 to +418

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Failure diagnosis can be attributed to the wrong host when connect calls race between URLs. The preflight callback stores _lastProbe unconditionally before the URL/state guard, so stale probe data survives even when that socket open is skipped.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 428:

<comment>Failure diagnosis can be attributed to the wrong host when connect calls race between URLs. The preflight callback stores `_lastProbe` unconditionally before the URL/state guard, so stale probe data survives even when that socket open is skipped.</comment>

<file context>
@@ -395,14 +420,14 @@ export function connect(
+	if (isRelayHostUrl(actualUrl)) {
+		transport._lastProbe = null;
+		void primeRelayAffinity(actualUrl).then((probe) => {
+			transport._lastProbe = probe;
+			openSocket();
+		});
</file context>
Suggested change
transport._lastProbe = probe;
openSocket();
if (
transport.currentUrl !== wsUrl ||
transport.connectionState !== "connecting"
) {
return;
}
transport._lastProbe = probe;
openSocket();

});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The preflight probe is fired against actualUrl, which may include ?replay=0 when the terminal has previously received bytes. primeRelayAffinity preserves all search params, so the _whoowns request becomes /_whoowns?replay=0&token=.... The relay likely ignores replay, but the probe should be authenticating with only the token — passing the WS-specific hint to an HTTP probe endpoint is unintentional.

Suggested change
if (isRelayHostUrl(actualUrl)) {
transport._lastProbe = null;
void primeRelayAffinity(actualUrl).then((probe) => {
transport._lastProbe = probe;
openSocket();
});
if (isRelayHostUrl(actualUrl)) {
transport._lastProbe = null;
void primeRelayAffinity(wsUrl).then((probe) => {
transport._lastProbe = probe;
openSocket();
});
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Line: 425-430

Comment:
The preflight probe is fired against `actualUrl`, which may include `?replay=0` when the terminal has previously received bytes. `primeRelayAffinity` preserves all search params, so the `_whoowns` request becomes `/_whoowns?replay=0&token=...`. The relay likely ignores `replay`, but the probe should be authenticating with only the token — passing the WS-specific hint to an HTTP probe endpoint is unintentional.

```suggestion
	if (isRelayHostUrl(actualUrl)) {
		transport._lastProbe = null;
		void primeRelayAffinity(wsUrl).then((probe) => {
			transport._lastProbe = probe;
			openSocket();
		});
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

} else {
openSocket();
}
Expand Down Expand Up @@ -486,11 +506,35 @@ function attachSocketListeners(
!transport._reconnectTimer &&
Boolean(transport.currentUrl && transport._terminal) &&
transport._reconnectAttempt < MAX_RECONNECT_ATTEMPTS;
pushLog(
transport,
willReconnect ? "warn" : "error",
`WebSocket closed while connected to ${formatWsEndpoint(transport.currentUrl)} (${formatCloseDetails(event)}). ${willReconnect ? "Reconnecting..." : "Max reconnect attempts reached."}`,
);
const endpoint = formatWsEndpoint(transport.currentUrl);
if (willReconnect) {
pushLog(
transport,
"warn",
`WebSocket closed while connected to ${endpoint} (${formatCloseDetails(event)}). Reconnecting...`,
);
} else {
// Gave up. Classify why from the preflight probe and record it so
// this failure mode is queryable, not silent.
const diagnosis = classifyTerminalFailure(
transport._lastProbe,
isRelayHostUrl(transport.currentUrl),
);
pushLog(
transport,
"error",
`WebSocket closed while connected to ${endpoint} (${formatCloseDetails(event)}). Max reconnect attempts reached. ${diagnosis.message}`,
);
posthog.capture("terminal_connect_failed", {
endpoint,
close_code: event.code,
close_reason: event.reason || undefined,
preflight_status: transport._lastProbe?.status ?? null,
tunnel_region: transport._lastProbe?.region ?? null,
reconnect_attempts: transport._reconnectAttempt,
category: diagnosis.category,
});
}
}
// Auto-reconnect on unexpected close (host-service restart, network blip)
scheduleReconnect(transport);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { describe, expect, it } from "bun:test";
import { classifyTerminalFailure } from "./terminalConnectionDiagnostics";

describe("classifyTerminalFailure", () => {
it("does not guess a cause for local (non-host) terminals", () => {
expect(
classifyTerminalFailure({ status: 200, region: "iad" }, false),
).toMatchObject({ category: "unknown" });
});

it("reports relay-unreachable when the preflight itself failed", () => {
expect(classifyTerminalFailure(null, true)).toMatchObject({
category: "relay-unreachable",
});
});

it("reports host-offline on 503", () => {
expect(
classifyTerminalFailure({ status: 503, region: null }, true),
).toMatchObject({ category: "host-offline" });
});

it("reports unauthorized on 401/403", () => {
expect(
classifyTerminalFailure({ status: 401, region: null }, true),
).toMatchObject({ category: "unauthorized" });
expect(
classifyTerminalFailure({ status: 403, region: null }, true),
).toMatchObject({ category: "unauthorized" });
});

it("reports stream-blocked when the host is present (200) but the WS still drops", () => {
const result = classifyTerminalFailure(
{ status: 200, region: "iad" },
true,
);
expect(result.category).toBe("stream-blocked");
// The region is surfaced so cross-region routing is obvious.
expect(result.message).toContain("iad");
});

it("falls back to unknown for unexpected statuses", () => {
expect(
classifyTerminalFailure({ status: 500, region: null }, true),
).toMatchObject({ category: "unknown" });
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import type { RelayAffinityProbe } from "@superset/workspace-client";

export type TerminalFailureCategory =
| "relay-unreachable"
| "host-offline"
| "unauthorized"
| "stream-blocked"
| "unknown";

export interface TerminalFailureClassification {
category: TerminalFailureCategory;
/** Short, user-facing reason for the terminal not connecting. */
message: string;
}

/**
* Turn the `_whoowns` preflight result (see `primeRelayAffinity`) into a cause
* for a failed terminal WS. The key case is `stream-blocked`: the host tunnel
* is present (probe 200) yet the WS still drops — the fingerprint of a relay
* routing problem (e.g. the cross-region 6PN path), not an offline host.
*/
export function classifyTerminalFailure(
probe: RelayAffinityProbe | null,
isHostUrl: boolean,
): TerminalFailureClassification {
// Local / same-machine terminals never hit the relay; don't guess a cause.
if (!isHostUrl) {
return {
category: "unknown",
message: "The terminal connection was lost.",
};
}
if (!probe) {
return {
category: "relay-unreachable",
message:
"Couldn't reach the relay service. Check your network connection.",
};
}
if (probe.status === 503) {
return {
category: "host-offline",
message: "This host is offline (not connected to the relay).",
};
}
Comment on lines +38 to +43

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The classifier handles 503 (host offline) but drops 502 into the unknown catchall with a generic "relay status 502" message. A relay 502 means it received a bad/no response from the upstream host service — semantically close to 503 and actionable. Users hitting a temporary host-service crash would see a cryptic status code rather than a helpful hint.

Suggested change
if (probe.status === 503) {
return {
category: "host-offline",
message: "This host is offline (not connected to the relay).",
};
}
if (probe.status === 502 || probe.status === 503) {
return {
category: "host-offline",
message: "This host is offline (not connected to the relay).",
};
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminalConnectionDiagnostics.ts
Line: 44-49

Comment:
The classifier handles 503 (host offline) but drops 502 into the `unknown` catchall with a generic "relay status 502" message. A relay 502 means it received a bad/no response from the upstream host service — semantically close to 503 and actionable. Users hitting a temporary host-service crash would see a cryptic status code rather than a helpful hint.

```suggestion
	if (probe.status === 502 || probe.status === 503) {
		return {
			category: "host-offline",
			message: "This host is offline (not connected to the relay).",
		};
	}
```

How can I resolve this? If you propose a fix, please make it concise.

if (probe.status === 401 || probe.status === 403) {
return {
category: "unauthorized",
message: "You don't have access to this host.",
};
}
if (probe.status === 200) {
const where = probe.region ? ` (region ${probe.region})` : "";
return {
category: "stream-blocked",
message: `The host is online${where} but the terminal stream couldn't connect. This is usually a relay routing issue, not the host.`,
};
}
return {
category: "unknown",
message: `The terminal connection failed (relay status ${probe.status}).`,
};
}
5 changes: 4 additions & 1 deletion packages/workspace-client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ export {
type WorkspaceChangedPayload,
type WorkspaceSnapshotPayload,
} from "./lib/eventBus";
export { primeRelayAffinity } from "./lib/primeRelayAffinity";
export {
primeRelayAffinity,
type RelayAffinityProbe,
} from "./lib/primeRelayAffinity";
export {
useWorkspaceClient,
useWorkspaceHostUrl,
Expand Down
54 changes: 54 additions & 0 deletions packages/workspace-client/src/lib/primeRelayAffinity.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { afterEach, expect, test } from "bun:test";
import { primeRelayAffinity } from "./primeRelayAffinity";

const realFetch = globalThis.fetch;
afterEach(() => {
globalThis.fetch = realFetch;
});

test("returns null (no probe) for non-/hosts URLs without fetching", async () => {
globalThis.fetch = (() => {
throw new Error("should not fetch");
}) as typeof fetch;
expect(await primeRelayAffinity("wss://relay.superset.sh/ws")).toBeNull();
});

test("probes /_whoowns keeping the token, and parses status + region", async () => {
let calledUrl = "";
globalThis.fetch = (async (input: string | URL | Request) => {
calledUrl = String(input);
return new Response(JSON.stringify({ ok: true, region: "iad" }), {
status: 200,
});
}) as typeof fetch;

const probe = await primeRelayAffinity(
"wss://relay.superset.sh/hosts/org:host/terminal/t1?token=abc",
);

expect(probe).toEqual({ status: 200, region: "iad" });
expect(calledUrl).toContain("/hosts/org:host/_whoowns");
expect(calledUrl).toContain("token=abc"); // auth query preserved
expect(calledUrl.startsWith("https://")).toBe(true); // wss -> https
});

test("surfaces a 503 as host-offline signal (status 503, no region)", async () => {
globalThis.fetch = (async () =>
new Response(JSON.stringify({ error: "Host not connected" }), {
status: 503,
})) as typeof fetch;
expect(
await primeRelayAffinity("wss://relay.superset.sh/hosts/h/events?token=x"),
).toEqual({ status: 503, region: null });
});

test("returns null when the relay is unreachable", async () => {
globalThis.fetch = (async () => {
throw new Error("network down");
}) as typeof fetch;
expect(
await primeRelayAffinity(
"wss://relay.superset.sh/hosts/h/terminal/t?token=x",
),
).toBeNull();
});
68 changes: 49 additions & 19 deletions packages/workspace-client/src/lib/primeRelayAffinity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,33 +7,63 @@
* edge affinity to the owning machine for subsequent requests, so the
* follow-up WS upgrade lands on the right instance and gets a clean 101.
*
* Best-effort: if the probe fails or times out, we still try the WS — it
* just may briefly flicker during the implicit retry.
* The response also doubles as a cheap health probe: because the WS API
* hides the upgrade's HTTP status (a 502/503 just surfaces as close code
* 1006), the returned status is the only client-visible signal for *why* a
* terminal/events stream can't connect. Callers use it to distinguish
* host-offline (503) from a routing failure (200 but the WS still drops)
* from an auth problem (401/403).
*
* Best-effort: if the probe fails or times out, we return null and still
* try the WS — it just may briefly flicker during the implicit retry.
*/

const PROBE_TIMEOUT_MS = 3_000;

export async function primeRelayAffinity(wsUrl: string): Promise<void> {
export interface RelayAffinityProbe {
/** HTTP status of the `_whoowns` preflight: 200 (host tunnel present),
* 503 (host not connected), 401/403 (unauthorized). */
status: number;
/** Relay region that owns the host tunnel, when the endpoint reports it. */
region: string | null;
}

export async function primeRelayAffinity(
wsUrl: string,
): Promise<RelayAffinityProbe | null> {
let url: URL;
try {
const url = new URL(wsUrl);
const match = url.pathname.match(/^(\/hosts\/[^/]+)/);
if (!match) return; // not a /hosts/<id>/* URL — nothing to prime
url.pathname = `${match[1]}/_whoowns`;
url.protocol = url.protocol === "wss:" ? "https:" : "http:";
// Keep search (token query param) so the relay can authenticate.
url = new URL(wsUrl);
} catch {
return null;
}
const match = url.pathname.match(/^(\/hosts\/[^/]+)/);
if (!match) return null; // not a /hosts/<id>/* URL — nothing to prime

url.pathname = `${match[1]}/_whoowns`;
url.protocol = url.protocol === "wss:" ? "https:" : "http:";
// Keep search (token query param) so the relay can authenticate.

const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), PROBE_TIMEOUT_MS);
const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), PROBE_TIMEOUT_MS);
try {
const res = await fetch(url.toString(), {
method: "GET",
signal: controller.signal,
cache: "no-store",
});
let region: string | null = null;
try {
await fetch(url.toString(), {
method: "GET",
signal: controller.signal,
cache: "no-store",
});
} finally {
clearTimeout(timer);
const body = (await res.json()) as { region?: unknown };
if (typeof body?.region === "string") region = body.region;
} catch {
// Error statuses may carry an empty / non-JSON body.
}
return { status: res.status, region };
} catch {
// Best-effort.
// Network error / timeout — the relay itself is unreachable.
return null;
} finally {
clearTimeout(timer);
}
}
Loading