Skip to content

Commit 447b18b

Browse files
authored
fix packaged mac relaunch blocked by obsolete outer (#5940)
1 parent 1226773 commit 447b18b

10 files changed

Lines changed: 533 additions & 3 deletions

File tree

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
export type DesktopExternalShowOptions = {
2+
onError?: (error: unknown) => void;
3+
};
4+
5+
/**
6+
* Notify an optional host after the desktop has accepted an external SHOW.
7+
* The callback starts immediately after focus so a packaged host can minimize
8+
* the interval in which an obsolete caller remains alive. Its asynchronous
9+
* completion does not delay the SHOW acknowledgement.
10+
*/
11+
export function notifyDesktopExternalShow(
12+
callback: (() => void | Promise<void>) | undefined,
13+
options: DesktopExternalShowOptions = {},
14+
): void {
15+
if (callback == null) return;
16+
const onError = options.onError ?? ((error: unknown) => {
17+
console.error("desktop external SHOW callback failed", error);
18+
});
19+
try {
20+
void Promise.resolve(callback()).catch(onError);
21+
} catch (error) {
22+
onError(error);
23+
}
24+
}

apps/desktop/src/main/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ import {
6363
exportDiagnosticsToFile,
6464
registerDesktopDiagnosticsIpc,
6565
} from "./diagnostics.js";
66+
import { notifyDesktopExternalShow } from "./external-show.js";
6667

6768
// Re-export pure URL-policy helpers so the packaged workspace's
6869
// vitest can pin their behaviour without spinning up a full Electron
@@ -141,6 +142,7 @@ export function applyOsLocaleSwitch(electronApp: Electron.App): string {
141142

142143
export type DesktopMainOptions = {
143144
beforeShutdown?: () => Promise<void>;
145+
onExternalShow?: () => void | Promise<void>;
144146
discoverWebUrl?: () => Promise<string | null>;
145147
/**
146148
* Round-7 (lefarcen P2 @ runtime.ts:336): packaged builds report the
@@ -866,6 +868,7 @@ export async function runDesktopMain(
866868
return activeDesktop.console();
867869
case SIDECAR_MESSAGES.SHOW:
868870
activeDesktop.show();
871+
notifyDesktopExternalShow(options.onExternalShow);
869872
return { accepted: true };
870873
case SIDECAR_MESSAGES.CLICK:
871874
return await activeDesktop.click(request.input as DesktopClickInput);
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
3+
import { notifyDesktopExternalShow } from "../../src/main/external-show.js";
4+
5+
describe("notifyDesktopExternalShow", () => {
6+
it("starts the packaged compatibility callback immediately without awaiting it", () => {
7+
let finish: (() => void) | undefined;
8+
const callback = vi.fn(() => new Promise<void>((resolve) => {
9+
finish = resolve;
10+
}));
11+
12+
notifyDesktopExternalShow(callback);
13+
14+
expect(callback).toHaveBeenCalledOnce();
15+
finish?.();
16+
});
17+
18+
it("contains callback failures at the optional host boundary", async () => {
19+
const onError = vi.fn();
20+
21+
notifyDesktopExternalShow(async () => {
22+
throw new Error("retirement failed");
23+
}, {
24+
onError,
25+
});
26+
27+
await vi.waitFor(() => expect(onError).toHaveBeenCalledWith(expect.objectContaining({
28+
message: "retirement failed",
29+
})));
30+
});
31+
32+
it("contains synchronous callback failures", () => {
33+
const onError = vi.fn();
34+
35+
notifyDesktopExternalShow(() => {
36+
throw new Error("retirement failed synchronously");
37+
}, { onError });
38+
39+
expect(onError).toHaveBeenCalledWith(expect.objectContaining({
40+
message: "retirement failed synchronously",
41+
}));
42+
});
43+
44+
it("does nothing when the packaged host did not opt in", () => {
45+
const onError = vi.fn();
46+
47+
notifyDesktopExternalShow(undefined, { onError });
48+
49+
expect(onError).not.toHaveBeenCalled();
50+
});
51+
});

apps/desktop/tests/main/updater-host-boundary.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,21 @@ describe("desktop updater host boundary", () => {
4646
expect(startupIpcBody).toContain("desktop runtime is not initialized");
4747
});
4848

49+
it("keeps obsolete installed-outer policy outside generic desktop while exposing the SHOW hook", () => {
50+
const main = source("src/main/index.ts");
51+
const showStart = main.indexOf("case SIDECAR_MESSAGES.SHOW:");
52+
const clickStart = main.indexOf("case SIDECAR_MESSAGES.CLICK:", showStart);
53+
expect(showStart).toBeGreaterThanOrEqual(0);
54+
expect(clickStart).toBeGreaterThan(showStart);
55+
const showHandler = main.slice(showStart, clickStart);
56+
expect(showHandler).toContain("activeDesktop.show()");
57+
expect(showHandler).toContain("notifyDesktopExternalShow(options.onExternalShow)");
58+
expect(showHandler.indexOf("activeDesktop.show()"))
59+
.toBeLessThan(showHandler.indexOf("notifyDesktopExternalShow(options.onExternalShow)"));
60+
expect(main).not.toContain("listProcessSnapshots");
61+
expect(main).not.toContain("stopProcesses");
62+
});
63+
4964
it("keeps desktop STATUS responsive when updater status is slow", () => {
5065
const main = source("src/main/index.ts");
5166
expect(main).toContain("async function snapshotUpdateForStatus()");

apps/packaged/src/index.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ import { startPackagedSidecars } from "./sidecars.js";
4949
import { reportStartupFailure, resolveStartupDistinctId } from "./startup-telemetry.js";
5050
import { resolvePackagedWindowTitle } from "./window-title.js";
5151
import { syncWindowsUninstallDisplayVersion } from "./windows-lifecycle.js";
52+
import { createObsoleteInstalledOuterRetirement } from "./obsolete-installed-outer.js";
5253

5354
let packagedLogger: PackagedDesktopLogger | null = null;
5455
let pendingSecondInstanceFocus = false;
@@ -174,6 +175,15 @@ async function main(): Promise<void> {
174175
});
175176
packagedLogger = createPackagedDesktopLogger(paths);
176177
attachPackagedDesktopProcessLogging({ logger: packagedLogger, paths, stamp });
178+
const retireObsoleteInstalledOuter = createObsoleteInstalledOuterRetirement({
179+
currentExecutablePath: process.execPath,
180+
currentPid: process.pid,
181+
installedLaunchPath: launcherRuntime.installedLaunchPath,
182+
logger: packagedLogger,
183+
payloadDesktopProcess: launcherRuntime.payloadDesktopProcess,
184+
payloadExecutablePath: launcherRuntime.desktopExecutablePath,
185+
platform: process.platform,
186+
});
177187
applyPackagedElectronPathOverrides(paths);
178188
applyPackagedUpdaterEnv(activeConfig.updateMetadataUrl);
179189
if (!claimPackagedSingleInstanceLock(app, () => {
@@ -259,9 +269,13 @@ async function main(): Promise<void> {
259269
splashStartedAt: splash.startedAt,
260270
async beforeShutdown() {
261271
try {
262-
await sidecars.close();
272+
await retireObsoleteInstalledOuter();
263273
} finally {
264-
await identity.close();
274+
try {
275+
await sidecars.close();
276+
} finally {
277+
await identity.close();
278+
}
265279
}
266280
},
267281
async discoverWebUrl() {
@@ -275,6 +289,9 @@ async function main(): Promise<void> {
275289
return sidecars.daemon.url;
276290
},
277291
windowTitle: resolvePackagedWindowTitle(activeConfig),
292+
async onExternalShow() {
293+
await retireObsoleteInstalledOuter();
294+
},
278295
onDesktopReady(controls) {
279296
void confirmPackagedLauncherRuntime(launcherRuntime).catch((error: unknown) => {
280297
packagedLogger?.warn("failed to confirm packaged launcher runtime", { error });
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
import { lstat } from "node:fs/promises";
2+
import { posix } from "node:path";
3+
4+
import {
5+
collectProcessTreePids,
6+
listProcessSnapshots,
7+
processCommandExactlyRunsExecutable,
8+
stopProcesses,
9+
type StopProcessesResult,
10+
} from "@open-design/platform";
11+
12+
type RetirementLogger = {
13+
info(message: string, meta?: Record<string, unknown>): void;
14+
warn(message: string, meta?: Record<string, unknown>): void;
15+
};
16+
17+
export type ObsoleteInstalledOuterRetirementContext = {
18+
currentExecutablePath: string;
19+
currentPid: number;
20+
installedLaunchPath: string | null;
21+
logger: RetirementLogger;
22+
payloadDesktopProcess: boolean;
23+
payloadExecutablePath: string | null;
24+
platform: NodeJS.Platform;
25+
};
26+
27+
type ObsoleteInstalledOuterRetirementDeps = {
28+
listProcessSnapshots?: typeof listProcessSnapshots;
29+
stopProcesses?: typeof stopProcesses;
30+
};
31+
32+
export type ObsoleteInstalledOuterRetirementResult =
33+
| {
34+
reason:
35+
| "invalid-install-anchor"
36+
| "not-payload-desktop"
37+
| "same-executable"
38+
| "unsupported-platform"
39+
| "unsafe-current-descendant";
40+
status: "skipped";
41+
}
42+
| {
43+
executablePath: string;
44+
reason: "no-match";
45+
status: "skipped";
46+
}
47+
| {
48+
executablePath: string;
49+
result: StopProcessesResult;
50+
rootPids: number[];
51+
status: "failed" | "retired";
52+
treePids: number[];
53+
};
54+
55+
function sameExecutablePath(left: string, right: string): boolean {
56+
return posix.normalize(left) === posix.normalize(right);
57+
}
58+
59+
async function resolveInstalledOuterExecutable(
60+
installedLaunchPath: string | null,
61+
platform: NodeJS.Platform,
62+
): Promise<string | null> {
63+
if (installedLaunchPath == null || installedLaunchPath.length === 0) return null;
64+
if (platform !== "darwin" || !posix.isAbsolute(installedLaunchPath)) return null;
65+
66+
const launchEntry = await lstat(installedLaunchPath).catch(() => null);
67+
if (launchEntry == null || launchEntry.isSymbolicLink()) return null;
68+
69+
if (!launchEntry.isDirectory() || !installedLaunchPath.endsWith(".app")) return null;
70+
const appName = posix.basename(installedLaunchPath, ".app");
71+
const executablePath = posix.join(installedLaunchPath, "Contents", "MacOS", appName);
72+
73+
const executableEntry = await lstat(executablePath).catch(() => null);
74+
if (executableEntry == null || !executableEntry.isFile() || executableEntry.isSymbolicLink()) return null;
75+
return executablePath;
76+
}
77+
78+
async function retireObsoleteInstalledOuter(
79+
context: ObsoleteInstalledOuterRetirementContext,
80+
deps: ObsoleteInstalledOuterRetirementDeps,
81+
): Promise<ObsoleteInstalledOuterRetirementResult> {
82+
if (!context.payloadDesktopProcess || context.payloadExecutablePath == null || !sameExecutablePath(
83+
context.currentExecutablePath,
84+
context.payloadExecutablePath,
85+
)) {
86+
return { reason: "not-payload-desktop", status: "skipped" };
87+
}
88+
if (context.platform !== "darwin") {
89+
return { reason: "unsupported-platform", status: "skipped" };
90+
}
91+
92+
const executablePath = await resolveInstalledOuterExecutable(context.installedLaunchPath, context.platform);
93+
if (executablePath == null) return { reason: "invalid-install-anchor", status: "skipped" };
94+
if (sameExecutablePath(executablePath, context.currentExecutablePath)) {
95+
return { reason: "same-executable", status: "skipped" };
96+
}
97+
98+
const snapshots = await (deps.listProcessSnapshots ?? listProcessSnapshots)();
99+
const rootPids = snapshots
100+
.filter((snapshot) => snapshot.pid !== context.currentPid && processCommandExactlyRunsExecutable(
101+
snapshot.command,
102+
executablePath,
103+
"darwin",
104+
))
105+
.map((snapshot) => snapshot.pid)
106+
.sort((left, right) => right - left);
107+
if (rootPids.length === 0) return { executablePath, reason: "no-match", status: "skipped" };
108+
109+
const safeRootPids = rootPids.filter((rootPid) => {
110+
const tree = collectProcessTreePids(snapshots, [rootPid]);
111+
return !tree.includes(context.currentPid);
112+
});
113+
if (safeRootPids.length === 0) {
114+
context.logger.warn("skipped obsolete installed outer retirement because it contains current payload", {
115+
currentPid: context.currentPid,
116+
executablePath,
117+
rootPids,
118+
});
119+
return { reason: "unsafe-current-descendant", status: "skipped" };
120+
}
121+
122+
const treePids = collectProcessTreePids(snapshots, safeRootPids);
123+
const result = await (deps.stopProcesses ?? stopProcesses)(treePids);
124+
const status = result.remainingPids.length === 0 ? "retired" : "failed";
125+
const meta = {
126+
executablePath,
127+
forcedPids: result.forcedPids,
128+
remainingPids: result.remainingPids,
129+
rootPids: safeRootPids,
130+
stoppedPids: result.stoppedPids,
131+
treePids,
132+
};
133+
if (status === "retired") {
134+
context.logger.info("retired obsolete installed outer", meta);
135+
} else {
136+
context.logger.warn("obsolete installed outer survived retirement", meta);
137+
}
138+
return { executablePath, result, rootPids: safeRootPids, status, treePids };
139+
}
140+
141+
/**
142+
* Build a re-usable, single-flight cleanup callback for desktop SHOW and quit.
143+
* A later invocation starts a fresh scan so a later LaunchServices open is not
144+
* hidden by a previously successful retirement.
145+
*/
146+
export function createObsoleteInstalledOuterRetirement(
147+
context: ObsoleteInstalledOuterRetirementContext,
148+
deps: ObsoleteInstalledOuterRetirementDeps = {},
149+
): () => Promise<ObsoleteInstalledOuterRetirementResult> {
150+
let pending: Promise<ObsoleteInstalledOuterRetirementResult> | null = null;
151+
return () => {
152+
if (pending != null) return pending;
153+
pending = retireObsoleteInstalledOuter(context, deps).finally(() => {
154+
pending = null;
155+
});
156+
return pending;
157+
};
158+
}

0 commit comments

Comments
 (0)