History

desktop: cover the sidecar exit and reconnect wiring, not just the classifiers

Rhys Sullivan committed 2 months ago

Commit bd63e401 parent 14091f33 files · +332 −1Browse files

Unified · Split

3 files
  1. apps/desktop/src/main/sidecar-exit-wiring.test.ts +163 −0
  2. packages/app/src/server-disconnected.test.ts +162 −0
  3. packages/app/src/server-disconnected.ts +7 −1
apps/desktop/src/main/sidecar-exit-wiring.test.ts+163−0
@@ -0,0 +1,163 @@
1+/**
2+ * The classifier is pure and tested next door; this covers what `startSidecar`
3+ * DOES with each verdict — which is where the reported symptom lived. A child
4+ * interrupted from outside must leave the crash channel untouched and tell the
5+ * window no report was sent, while a genuine post-boot death must still be
6+ * reported. Both halves are driven through the real `startSidecar`, with only
7+ * the process spawn and Electron's environment replaced.
8+ */
9+
10+import { describe, expect, it } from "@effect/vitest";
11+// oxlint-disable-next-line executor/no-vitest-import -- boundary: vi.mock must come from vitest itself for mock hoisting to resolve
12+import { vi } from "vitest";
13+import { EventEmitter } from "node:events";
14+
15+let appQuitting = false;
16+const crashReports: string[] = [];
17+
18+vi.mock("electron", () => ({
19+ app: { isPackaged: true, getVersion: () => "1.6.0", on: () => {} },
20+}));
21+
22+vi.mock("electron-log/main.js", () => {
23+ const scope = () => ({ info: () => {}, error: () => {}, warn: () => {}, debug: () => {} });
24+ return { default: { scope }, scope };
25+});
26+
27+vi.mock("./local-auth", () => ({ loadOrMintLocalAuthToken: () => "test-token" }));
28+
29+vi.mock("./settings", () => ({ getServerSettings: () => ({ port: 4789 }) }));
30+
31+vi.mock("./diagnostics", () => ({
32+ reportSidecarCrash: (message: string) => {
33+ crashReports.push(message);
34+ },
35+ sidecarCrashReportingEnv: () => ({}),
36+}));
37+
38+vi.mock("./supervised-daemon", () => ({ resolveSupervisedDaemonAttach: () => null }));
39+
40+vi.mock("./app-quit", () => ({ isAppQuitting: () => appQuitting }));
41+
42+class FakeChild extends EventEmitter {
43+ readonly stdout = new EventEmitter();
44+ readonly stderr = new EventEmitter();
45+ readonly pid = 4242;
46+ exitCode: number | null = null;
47+ killed = false;
48+ kill() {
49+ this.killed = true;
50+ return true;
51+ }
52+}
53+
54+// The packaged branch of `resolveSidecarCommand` reads Electron's resources
55+// path; nothing is executed, since the spawn itself is replaced below.
56+(process as { resourcesPath?: string }).resourcesPath ??= "/tmp/executor-test-resources";
57+
58+let spawned: FakeChild | null = null;
59+
60+vi.mock("node:child_process", () => ({
61+ spawn: () => {
62+ spawned = new FakeChild();
63+ return spawned;
64+ },
65+}));
66+
67+const { startSidecar, onUnexpectedSidecarExit, stopSidecar } = await import("./sidecar");
68+
69+/** Boot a sidecar to the point `startSidecar` resolves, then hand back the child. */
70+const bootedSidecar = async (): Promise<FakeChild> => {
71+ const started = startSidecar();
72+ // The child announces readiness with the structured stdout sentinel.
73+ await Promise.resolve();
74+ spawned?.stdout.emit("data", Buffer.from("EXECUTOR_READY:4789\n"));
75+ await started;
76+ // oxlint-disable-next-line executor/no-non-null-assertion -- the spawn mock always records a child
77+ return spawned!;
78+};
79+
80+interface Observed {
81+ readonly notices: { readonly reported: boolean }[];
82+ readonly reports: string[];
83+}
84+
85+/** Boot, drive one post-boot exit, and report what the app did about it. */
86+const exitAfterBoot = async (
87+ exit: { readonly code: number | null; readonly signal: NodeJS.Signals | null },
88+ options: { readonly quitting?: boolean; readonly stopFirst?: boolean } = {},
89+): Promise<Observed> => {
90+ appQuitting = false;
91+ crashReports.length = 0;
92+ const notices: { readonly reported: boolean }[] = [];
93+ onUnexpectedSidecarExit((notice) => notices.push(notice));
94+
95+ const child = await bootedSidecar();
96+ child.stderr.emit("data", Buffer.from("some trailing output\n"));
97+ if (options.stopFirst) {
98+ const stopping = stopSidecar(child as never);
99+ child.emit("exit", exit.code, exit.signal);
100+ await stopping;
101+ } else {
102+ appQuitting = options.quitting ?? false;
103+ child.emit("exit", exit.code, exit.signal);
104+ }
105+ await Promise.resolve();
106+ return { notices, reports: [...crashReports] };
107+};
108+
109+describe("startSidecar post-boot exit handling", () => {
110+ it("does not report an outside interrupt, and tells the window nothing was sent", async () => {
111+ // A group-wide Ctrl-C reaches the child because it shares Electron's
112+ // process group; Node surfaces it as 128+SIGINT. The server really is gone,
113+ // so the window must still be told — but no crash report exists to claim.
114+ const { notices, reports } = await exitAfterBoot({ code: 130, signal: null });
115+
116+ expect(reports).toEqual([]);
117+ expect(notices).toEqual([{ reported: false }]);
118+ });
119+
120+ it("does not report a SIGTERM the app did not send", async () => {
121+ const { notices, reports } = await exitAfterBoot({ code: null, signal: "SIGTERM" });
122+
123+ expect(reports).toEqual([]);
124+ expect(notices).toEqual([{ reported: false }]);
125+ });
126+
127+ it("still reports a genuine post-boot death and says a report was sent", async () => {
128+ const { notices, reports } = await exitAfterBoot({ code: 1, signal: null });
129+
130+ expect(reports).toHaveLength(1);
131+ expect(reports[0]).toContain("code=1");
132+ expect(notices).toEqual([{ reported: true }]);
133+ });
134+
135+ it("still reports an abnormal death signal", async () => {
136+ const { notices, reports } = await exitAfterBoot({ code: null, signal: "SIGSEGV" });
137+
138+ expect(reports).toHaveLength(1);
139+ expect(notices).toEqual([{ reported: true }]);
140+ });
141+
142+ it("says nothing at all when the app itself stopped the sidecar", async () => {
143+ const { notices, reports } = await exitAfterBoot(
144+ { code: null, signal: "SIGTERM" },
145+ { stopFirst: true },
146+ );
147+
148+ expect(reports).toEqual([]);
149+ expect(notices).toEqual([]);
150+ });
151+
152+ it("says nothing at all when the app is quitting", async () => {
153+ // Quit tears the tree down; the window is going away, so surfacing a
154+ // disconnected screen at that moment would be its own bug.
155+ const { notices, reports } = await exitAfterBoot(
156+ { code: 130, signal: null },
157+ { quitting: true },
158+ );
159+
160+ expect(reports).toEqual([]);
161+ expect(notices).toEqual([]);
162+ });
163+});
packages/app/src/server-disconnected.test.ts+162−0
@@ -0,0 +1,162 @@
1+/**
2+ * The recovery policy is tested against a fake environment in
3+ * preload-error.test.ts; this covers the environment the browser actually gets,
4+ * because a policy fed the wrong inputs recovers from nothing. The failure this
5+ * guards against is silent: a probe that answers backwards, or a reload guard
6+ * that never latches, turns "reload once" into a reload loop with every
7+ * behavioural test still green.
8+ */
9+
10+import { describe, expect, it } from "@effect/vitest";
11+import { browserEnvironment } from "./server-disconnected";
12+
13+class FakeElement {
14+ id = "";
15+ textContent = "";
16+ readonly style = { cssText: "" };
17+ readonly children: FakeElement[] = [];
18+
19+ append(...nodes: FakeElement[]): void {
20+ this.children.push(...nodes);
21+ }
22+
23+ byId(id: string): FakeElement | null {
24+ if (this.id === id) return this;
25+ for (const child of this.children) {
26+ const found = child.byId(id);
27+ if (found) return found;
28+ }
29+ return null;
30+ }
31+
32+ get text(): string {
33+ return [this.textContent, ...this.children.map((child) => child.text)].join(" ").trim();
34+ }
35+}
36+
37+interface FetchCall {
38+ readonly input: unknown;
39+ readonly init: RequestInit | undefined;
40+}
41+
42+const environment = (options: {
43+ readonly respond?: () => Promise<{ ok: boolean; body: string }>;
44+ readonly storage?: "working" | "blocked";
45+}) => {
46+ const body = new FakeElement();
47+ const fetches: FetchCall[] = [];
48+ const reloads = { count: 0 };
49+ const reported: unknown[] = [];
50+ const store = new Map<string, string>();
51+
52+ const sessionStorage = {
53+ getItem: (key: string) => store.get(key) ?? null,
54+ setItem: (key: string, value: string) => store.set(key, value),
55+ };
56+
57+ const fakeWindow = {
58+ addEventListener: () => {},
59+ removeEventListener: () => {},
60+ location: {
61+ reload: () => {
62+ reloads.count += 1;
63+ },
64+ },
65+ };
66+ if (options.storage === "blocked") {
67+ Object.defineProperty(fakeWindow, "sessionStorage", {
68+ get: () => {
69+ // oxlint-disable-next-line executor/no-error-constructor, executor/no-try-catch-or-throw -- boundary: reproduces the SecurityError a browser raises when storage is blocked
70+ throw new Error("SecurityError: storage is disabled");
71+ },
72+ });
73+ } else {
74+ Object.defineProperty(fakeWindow, "sessionStorage", { get: () => sessionStorage });
75+ }
76+
77+ const globals = globalThis as Record<string, unknown>;
78+ globals.window = fakeWindow;
79+ globals.document = {
80+ body,
81+ createElement: () => new FakeElement(),
82+ getElementById: (id: string) => body.byId(id),
83+ };
84+ globals.fetch = async (input: unknown, init?: RequestInit) => {
85+ fetches.push({ input, init });
86+ const answer = await (options.respond?.() ?? Promise.resolve({ ok: true, body: "ok" }));
87+ return { ok: answer.ok, text: async () => answer.body };
88+ };
89+ globals.reportError = (error: unknown) => reported.push(error);
90+
91+ return { env: browserEnvironment(), body, fetches, reloads, reported, store };
92+};
93+
94+describe("browserEnvironment", () => {
95+ it("probes the unauthenticated health endpoint and believes only a healthy answer", async () => {
96+ const healthy = environment({ respond: async () => ({ ok: true, body: "ok\n" }) });
97+ expect(await healthy.env.probeServer()).toBe(true);
98+ expect(healthy.fetches).toHaveLength(1);
99+ expect(healthy.fetches[0]?.input).toBe("/api/health");
100+ // A cached 200 from before the server died would report it as alive.
101+ expect(healthy.fetches[0]?.init?.cache).toBe("no-store");
102+
103+ const wrongBody = environment({ respond: async () => ({ ok: true, body: "<!doctype html>" }) });
104+ expect(await wrongBody.env.probeServer()).toBe(false);
105+
106+ const failing = environment({ respond: async () => ({ ok: false, body: "" }) });
107+ expect(await failing.env.probeServer()).toBe(false);
108+ });
109+
110+ it("reads a refused connection as a down server rather than propagating", async () => {
111+ const down = environment({
112+ respond: async () => {
113+ // oxlint-disable-next-line executor/no-error-constructor, executor/no-try-catch-or-throw -- boundary: fetch rejects when the origin is gone, which is the signal under test
114+ throw new TypeError("Failed to fetch");
115+ },
116+ });
117+
118+ expect(await down.env.probeServer()).toBe(false);
119+ });
120+
121+ it("shows one reconnect overlay however many times it is asked", () => {
122+ const { env, body } = environment({});
123+
124+ env.showDisconnected();
125+ env.showDisconnected();
126+
127+ const overlays = body.children.filter((child) => child.id === "executor-server-disconnected");
128+ expect(overlays).toHaveLength(1);
129+ expect(overlays[0]?.text).toContain("Lost connection to the Executor server");
130+ expect(overlays[0]?.text).toContain("RECONNECTING");
131+ });
132+
133+ it("latches the one-reload-per-session guard in storage", () => {
134+ const { env, store } = environment({});
135+
136+ expect(env.readReloadFlag()).toBe(false);
137+ env.writeReloadFlag();
138+
139+ expect(env.readReloadFlag()).toBe(true);
140+ expect([...store.keys()]).toEqual(["executor:preload-reloaded"]);
141+ });
142+
143+ it("reports rather than reloads when storage is unavailable", () => {
144+ // Without a place to record the reload, an auto-reload could repeat
145+ // forever; reading the flag as already spent degrades to reporting once.
146+ const { env } = environment({ storage: "blocked" });
147+
148+ expect(env.readReloadFlag()).toBe(true);
149+ });
150+
151+ it("reloads the page and routes reports to the shared reporter", () => {
152+ const { env, reloads, reported } = environment({});
153+ // oxlint-disable-next-line executor/no-error-constructor -- test boundary: stands in for the chunk TypeError
154+ const failure = new TypeError("Failed to fetch dynamically imported module");
155+
156+ env.reload();
157+ env.report(failure);
158+
159+ expect(reloads.count).toBe(1);
160+ expect(reported).toEqual([failure]);
161+ });
162+});
packages/app/src/server-disconnected.ts+7−1
@@ -71,7 +71,13 @@ const sessionFlag = (): Pick<Storage, "getItem" | "setItem"> | null => { · 70 unmodified lines
7171 }
7272 };
7373
74−const browserEnvironment = (): PreloadErrorEnvironment => {
74+/**
75+ * The real browser half of the environment `respondToPreloadError` consumes.
76+ * Exported so a test can check the mapping itself — an inverted probe or a
77+ * reload guard that never latches would leave every behavioural test green
78+ * while the app reload-loops.
79+ */
80+export const browserEnvironment = (): PreloadErrorEnvironment => {
7581 const storage = sessionFlag();
7682 return {
7783 target: window,