Skip to content

Commit 21203e1

Browse files
pallyoungpallyoung
committed
Preserve terminals across managed restarts
1 parent a8fa9de commit 21203e1

6 files changed

Lines changed: 261 additions & 61 deletions

File tree

‎packages/cli/src/pm2-control.test.ts‎

Lines changed: 72 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,11 @@ describe("pm2-control", () => {
5151
return undefined;
5252
});
5353
start.mockImplementation(
54-
(_config: unknown, callback: (error: Error | null, apps: unknown[]) => void) => {
54+
(
55+
_script: string,
56+
_config: unknown,
57+
callback: (error: Error | null, apps: unknown[]) => void
58+
) => {
5559
writeRuntimeConfig({
5660
host: "127.0.0.1",
5761
port: 4187,
@@ -106,20 +110,19 @@ describe("pm2-control", () => {
106110
});
107111

108112
it("starts the managed server with the fixed app name", async () => {
113+
process.env.NODE_ENV = "development";
114+
109115
await startManagedServer({
110116
script: "/cli/dist/esm/server-runner.js",
111117
cwd: "/repo",
112118
waitMs: 10,
113119
});
114120

115121
expect(start).toHaveBeenCalledWith(
122+
"/cli/dist/esm/server-runner.js",
116123
expect.objectContaining({
117124
name: MANAGED_SERVER_NAME,
118-
script: "/cli/dist/esm/server-runner.js",
119125
cwd: "/repo",
120-
env: expect.objectContaining({
121-
NODE_ENV: "production",
122-
}),
123126
autorestart: true,
124127
restart_delay: 2000,
125128
min_uptime: "5s",
@@ -131,6 +134,54 @@ describe("pm2-control", () => {
131134
);
132135
});
133136

137+
it("temporarily forces NODE_ENV=production during PM2 startup without passing env opts", async () => {
138+
process.env.NODE_ENV = "development";
139+
140+
start.mockImplementationOnce(
141+
(
142+
_script: string,
143+
config: unknown,
144+
callback: (error: Error | null, apps: unknown[]) => void
145+
) => {
146+
expect(process.env.NODE_ENV).toBe("production");
147+
expect(config).not.toHaveProperty("env");
148+
writeRuntimeConfig({
149+
host: "127.0.0.1",
150+
port: 4187,
151+
pid: 424242,
152+
token: "test-token",
153+
serverInstanceId: "server-1",
154+
startedAt: Date.now(),
155+
});
156+
callback(null, [{ pm_id: 1 }]);
157+
}
158+
);
159+
160+
await startManagedServer({
161+
script: "/cli/dist/esm/server-runner.js",
162+
cwd: "/repo",
163+
waitMs: 10,
164+
});
165+
166+
expect(process.env.NODE_ENV).toBe("development");
167+
});
168+
169+
it("starts PM2 in script mode instead of JSON config mode", async () => {
170+
await startManagedServer({
171+
script: "/cli/dist/esm/server-runner.js",
172+
cwd: "/repo",
173+
waitMs: 10,
174+
});
175+
176+
expect(start).toHaveBeenCalledWith(
177+
"/cli/dist/esm/server-runner.js",
178+
expect.not.objectContaining({
179+
script: expect.anything(),
180+
}),
181+
expect.any(Function)
182+
);
183+
});
184+
134185
it("passes script args through for the managed server entrypoint", async () => {
135186
await startManagedServer({
136187
script: "/cli/dist/esm/server-runner.js",
@@ -140,8 +191,8 @@ describe("pm2-control", () => {
140191
});
141192

142193
expect(start).toHaveBeenCalledWith(
194+
"/cli/dist/esm/server-runner.js",
143195
expect.objectContaining({
144-
script: "/cli/dist/esm/server-runner.js",
145196
args: ["--flag"],
146197
}),
147198
expect.any(Function)
@@ -305,7 +356,11 @@ describe("pm2-control", () => {
305356

306357
it("fails background startup when runtime readiness times out", async () => {
307358
start.mockImplementationOnce(
308-
(_config: unknown, callback: (error: Error | null, apps: unknown[]) => void) => {
359+
(
360+
_script: string,
361+
_config: unknown,
362+
callback: (error: Error | null, apps: unknown[]) => void
363+
) => {
309364
callback(null, [{ pm_id: 1 }]);
310365
}
311366
);
@@ -340,7 +395,11 @@ describe("pm2-control", () => {
340395
writeFileSync(errFile, "Error: stale previous startup failure\n");
341396

342397
start.mockImplementationOnce(
343-
(config: unknown, callback: (error: Error | null, apps: unknown[]) => void) => {
398+
(
399+
_script: string,
400+
config: unknown,
401+
callback: (error: Error | null, apps: unknown[]) => void
402+
) => {
344403
const errorFile = (config as { error_file: string }).error_file;
345404
writeFileSync(
346405
errorFile,
@@ -374,6 +433,11 @@ describe("pm2-control", () => {
374433
});
375434

376435
it("maps missing PM2 app to stopped status", async () => {
436+
describeProcess.mockImplementationOnce(
437+
(_name: string, callback: (error: Error | null, result: unknown[]) => void) =>
438+
callback(null, [])
439+
);
440+
377441
await expect(getManagedServerStatus()).resolves.toEqual({
378442
status: "stopped",
379443
pm2Pid: null,

‎packages/cli/src/pm2-control.ts‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ type Pm2Module = {
7272
disconnect: (cb?: (err: Error | null, data?: unknown) => void) => void;
7373
describe: (name: string, cb: (err: Error | null, result: unknown[]) => void) => void;
7474
delete: (name: string, cb: (err: Error | null) => void) => void;
75-
start: (opts: unknown, cb: (err: Error | null) => void) => void;
75+
start: (script: string, opts: unknown, cb: (err: Error | null) => void) => void;
7676
kill: (cb: (err: Error | null) => void) => void;
7777
};
7878

@@ -389,35 +389,41 @@ export const startManagedServer = async ({
389389
ensureLogDirectory();
390390
const { outFile, errFile } = getLogPaths();
391391
const logOffsets = captureStartupLogOffsets();
392+
const previousNodeEnv = process.env.NODE_ENV;
393+
process.env.NODE_ENV = "production";
392394

393-
await new Promise<void>((resolve, reject) => {
394-
pm2.start(
395-
{
396-
name: MANAGED_SERVER_NAME,
395+
try {
396+
await new Promise<void>((resolve, reject) => {
397+
pm2.start(
397398
script,
398-
cwd,
399-
...(args !== undefined ? { args } : {}),
400-
env: {
401-
...process.env,
402-
NODE_ENV: "production",
399+
{
400+
name: MANAGED_SERVER_NAME,
401+
cwd,
402+
...(args !== undefined ? { args } : {}),
403+
autorestart: true,
404+
restart_delay: PM2_RESTART_DELAY_MS,
405+
min_uptime: PM2_MIN_UPTIME,
406+
max_restarts: PM2_MAX_RESTARTS,
407+
out_file: outFile,
408+
error_file: errFile,
403409
},
404-
autorestart: true,
405-
restart_delay: PM2_RESTART_DELAY_MS,
406-
min_uptime: PM2_MIN_UPTIME,
407-
max_restarts: PM2_MAX_RESTARTS,
408-
out_file: outFile,
409-
error_file: errFile,
410-
},
411-
(error) => {
412-
if (error) {
413-
reject(error);
414-
return;
415-
}
410+
(error) => {
411+
if (error) {
412+
reject(error);
413+
return;
414+
}
416415

417-
resolve();
418-
}
419-
);
420-
});
416+
resolve();
417+
}
418+
);
419+
});
420+
} finally {
421+
if (previousNodeEnv === undefined) {
422+
delete process.env.NODE_ENV;
423+
} else {
424+
process.env.NODE_ENV = previousNodeEnv;
425+
}
426+
}
421427

422428
await waitForRuntimeReady(pm2, waitMs, logOffsets);
423429
} catch (error) {

‎packages/cli/src/terminal-broker-control.process.test.ts‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
2+
import { tmpdir } from "node:os";
3+
import { join } from "node:path";
4+
import { deleteTerminalBrokerRuntime } from "@coder-studio/core/runtime";
5+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
6+
7+
vi.mock("@coder-studio/server", () => ({
8+
TerminalBrokerClient: class MockTerminalBrokerClient {
9+
async ping(): Promise<boolean> {
10+
return true;
11+
}
12+
},
13+
}));
14+
15+
import { ensureTerminalBroker } from "./terminal-broker-control.js";
16+
17+
describe("terminal-broker-control process isolation", () => {
18+
const originalHome = process.env.HOME;
19+
const originalUserProfile = process.env.USERPROFILE;
20+
const originalRuntimeDir = process.env.CODER_STUDIO_RUNTIME_DIR;
21+
22+
let testRootDir: string;
23+
let runtimeDir: string;
24+
let brokerScriptPath: string;
25+
let brokerInfoPath: string;
26+
let brokerPid: number | null;
27+
28+
beforeEach(() => {
29+
testRootDir = mkdtempSync(join(tmpdir(), "cs-terminal-broker-process-"));
30+
runtimeDir = join(testRootDir, "runtime");
31+
brokerScriptPath = join(testRootDir, "fake-broker.mjs");
32+
brokerInfoPath = join(testRootDir, "broker-process.json");
33+
brokerPid = null;
34+
35+
mkdirSync(runtimeDir, { recursive: true });
36+
process.env.HOME = testRootDir;
37+
process.env.USERPROFILE = testRootDir;
38+
process.env.CODER_STUDIO_RUNTIME_DIR = runtimeDir;
39+
deleteTerminalBrokerRuntime();
40+
41+
writeFileSync(
42+
brokerScriptPath,
43+
`
44+
import { mkdirSync, writeFileSync } from "node:fs";
45+
import { join } from "node:path";
46+
47+
const runtimeDir = process.env.CODER_STUDIO_RUNTIME_DIR;
48+
if (!runtimeDir) {
49+
throw new Error("Missing CODER_STUDIO_RUNTIME_DIR");
50+
}
51+
52+
mkdirSync(runtimeDir, { recursive: true });
53+
writeFileSync(
54+
join(runtimeDir, "terminal-broker.json"),
55+
JSON.stringify(
56+
{
57+
endpoint: process.env.CODER_STUDIO_TERMINAL_BROKER_ENDPOINT,
58+
pid: process.pid,
59+
startedAt: Date.now(),
60+
},
61+
null,
62+
2
63+
)
64+
);
65+
writeFileSync(
66+
${JSON.stringify(brokerInfoPath)},
67+
JSON.stringify({ pid: process.pid, ppid: process.ppid }, null, 2)
68+
);
69+
70+
setInterval(() => undefined, 1_000);
71+
`,
72+
"utf8"
73+
);
74+
});
75+
76+
afterEach(() => {
77+
if (brokerPid !== null) {
78+
try {
79+
process.kill(brokerPid, "SIGKILL");
80+
} catch {
81+
// Process already exited.
82+
}
83+
}
84+
85+
vi.restoreAllMocks();
86+
deleteTerminalBrokerRuntime();
87+
88+
if (originalHome === undefined) {
89+
delete process.env.HOME;
90+
} else {
91+
process.env.HOME = originalHome;
92+
}
93+
94+
if (originalUserProfile === undefined) {
95+
delete process.env.USERPROFILE;
96+
} else {
97+
process.env.USERPROFILE = originalUserProfile;
98+
}
99+
100+
if (originalRuntimeDir === undefined) {
101+
delete process.env.CODER_STUDIO_RUNTIME_DIR;
102+
} else {
103+
process.env.CODER_STUDIO_RUNTIME_DIR = originalRuntimeDir;
104+
}
105+
106+
rmSync(testRootDir, { recursive: true, force: true });
107+
});
108+
109+
it("launches the broker outside the current process tree", async () => {
110+
const runtime = await ensureTerminalBroker({
111+
script: brokerScriptPath,
112+
cwd: testRootDir,
113+
waitMs: 1_000,
114+
});
115+
brokerPid = runtime.pid;
116+
117+
const brokerInfo = JSON.parse(readFileSync(brokerInfoPath, "utf8")) as {
118+
pid: number;
119+
ppid: number;
120+
};
121+
122+
expect(brokerInfo.pid).toBe(runtime.pid);
123+
expect(brokerInfo.ppid).not.toBe(process.pid);
124+
});
125+
});

‎packages/cli/src/terminal-broker-control.test.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,13 @@ describe("terminal-broker-control", () => {
9494

9595
expect(spawn).toHaveBeenCalledWith(
9696
process.execPath,
97-
["/cli/dist/esm/terminal-broker-runner.mjs"],
97+
[
98+
"--input-type=module",
99+
"-e",
100+
expect.stringContaining('["/cli/dist/esm/terminal-broker-runner.mjs"]'),
101+
],
98102
expect.objectContaining({
103+
cwd: "/repo",
99104
detached: true,
100105
stdio: "ignore",
101106
})

0 commit comments

Comments
 (0)