Skip to content

Commit 8e17d7d

Browse files
committed
refactor(dev): address ARV review on dev-server-core
- port: reject non-EADDRINUSE errors instead of treating every failure as "port busy" (EACCES on a privileged port no longer masquerades as in-use and gets silently walked past) - dev: delete duplicate core/dev/run.ts; consume io/exec.ts (runProcess) so subprocess IO lives only in io/ (boundary of concern) - codezip: drop nodePackageManager lockfile detection; assume npm - process: rewrite watch() comment; tighten 128+signal comment
1 parent 2e1c2d1 commit 8e17d7d

6 files changed

Lines changed: 42 additions & 94 deletions

File tree

src/core/dev/codezip.test.ts

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,32 @@ import { join } from "node:path";
55
import type { ProjectRuntime } from "../../core/project/schema";
66
import type { StartDevServerInput, DevServerHandle } from "../../handlers/project/dev/types";
77
import { createSilentLogger } from "../../testing";
8-
import { CodeZipDevRunner, nodePackageManager, parseEntrypoint, serverCommand } from "./codezip";
8+
import { CodeZipDevRunner, parseEntrypoint, serverCommand } from "./codezip";
9+
10+
/** The schema brands entrypoint/codeLocation as FilePath/DirectoryPath; fixtures
11+
* supply plain strings and cast through the brand in one place. */
12+
function runtime(spec: {
13+
name: string;
14+
build: "CodeZip" | "Container";
15+
entrypoint: string;
16+
codeLocation: string;
17+
}): ProjectRuntime {
18+
return spec as ProjectRuntime;
19+
}
920

10-
const pythonRuntime: ProjectRuntime = {
21+
const pythonRuntime = runtime({
1122
name: "hello_world",
1223
build: "CodeZip",
1324
entrypoint: "main.py",
1425
codeLocation: "app/hello-world",
15-
};
26+
});
1627

17-
const tsRuntime: ProjectRuntime = {
28+
const tsRuntime = runtime({
1829
name: "hello_world",
1930
build: "CodeZip",
2031
entrypoint: "index.ts",
2132
codeLocation: "app/hello-world",
22-
};
33+
});
2334

2435
/** Accumulates run/spawn calls without executing anything real. */
2536
function harness() {
@@ -132,14 +143,14 @@ describe("CodeZipDevRunner", () => {
132143
await projectFile(root, "app/hello-world/.venv/bin", "uvicorn");
133144

134145
await runner.start(
135-
startInput(root, h, { ...pythonRuntime, entrypoint: "main.py:application" }),
146+
startInput(root, h, runtime({ ...pythonRuntime, entrypoint: "main.py:application" })),
136147
);
137148

138149
expect(h.spawned[0]).toContain("uvicorn");
139150
await rm(root, { recursive: true, force: true });
140151
});
141152

142-
test("installs node_modules via detected package manager for TypeScript runtime", async () => {
153+
test("installs node_modules with npm for TypeScript runtime", async () => {
143154
const h = harness();
144155
const runner = new CodeZipDevRunner({
145156
logger: createSilentLogger(),
@@ -149,11 +160,10 @@ describe("CodeZipDevRunner", () => {
149160

150161
const root = join(tmpdir(), `codezip-test-${Date.now()}`);
151162
await projectTree(root, "app/hello-world");
152-
await projectFile(root, "app/hello-world", "pnpm-lock.yaml");
153163

154164
await runner.start(startInput(root, h, tsRuntime));
155165

156-
expect(h.commands[0]?.[0]).toContain("pnpm");
166+
expect(h.commands[0]?.[0]).toContain("npm");
157167
await rm(root, { recursive: true, force: true });
158168
});
159169
});
@@ -164,26 +174,11 @@ describe("parseEntrypoint", () => {
164174
["main.py:application", "main.py", "application", "python"],
165175
["index.ts", "index.ts", "app", "typescript"],
166176
["src/server.ts:handler", "src/server.ts", "handler", "typescript"],
167-
])("parses %s", (input, file, handler, language) => {
177+
] as const)("parses %s", (input, file, handler, language) => {
168178
expect(parseEntrypoint(input)).toEqual({ file, handler, language });
169179
});
170180
});
171181

172-
describe("nodePackageManager", () => {
173-
test.each([
174-
["pnpm-lock.yaml", "pnpm"],
175-
["yarn.lock", "yarn"],
176-
["package-lock.json", "npm"],
177-
])("detects %s as %s", async (lockfile, expected) => {
178-
const root = join(tmpdir(), `pm-test-${Date.now()}`);
179-
await mkdir(root, { recursive: true });
180-
await writeFile(join(root, lockfile), "");
181-
182-
expect(nodePackageManager(root)).toBe(expected);
183-
await rm(root, { recursive: true, force: true });
184-
});
185-
});
186-
187182
describe("serverCommand", () => {
188183
test("builds uvicorn command for Python", () => {
189184
const cmd = serverCommand(

src/core/dev/codezip.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import { existsSync } from "node:fs";
22
import { join } from "node:path";
3-
import { ValidationError } from "../../errors";
3+
import { InputValidationError } from "../../errors";
44
import type {
55
DevRunner,
66
DevServerHandle,
77
StartDevServerInput,
88
} from "../../handlers/project/dev/types";
99
import type { Logger } from "../../logging";
10-
import { runCommand, type CommandRunner } from "./run";
10+
import { runProcess, type ProcessRunner } from "../../io";
1111
import { ProcessSupervisor, windowsExecutable, type ProcessCommand } from "./process";
1212

1313
/** An entrypoint interpreted exactly once: "main.py:application" is the file
@@ -28,17 +28,10 @@ export function parseEntrypoint(entrypoint: string): Entrypoint {
2828
};
2929
}
3030

31-
/** Detects the package manager for a Node project from its lockfile. */
32-
export function nodePackageManager(directory: string): "npm" | "pnpm" | "yarn" {
33-
if (existsSync(join(directory, "pnpm-lock.yaml"))) return "pnpm";
34-
if (existsSync(join(directory, "yarn.lock"))) return "yarn";
35-
return "npm";
36-
}
37-
3831
type CodeZipDevRunnerConfig = {
3932
logger: Logger;
4033
/** Injectable process seams so tests never spawn uv or a real server. */
41-
run?: CommandRunner;
34+
run?: ProcessRunner;
4235
supervisor?: ProcessSupervisor;
4336
};
4437

@@ -48,19 +41,19 @@ type CodeZipDevRunnerConfig = {
4841
*/
4942
export class CodeZipDevRunner implements DevRunner {
5043
private readonly logger: Logger;
51-
private readonly run: CommandRunner;
44+
private readonly run: ProcessRunner;
5245
private readonly supervisor: ProcessSupervisor;
5346

5447
constructor(config: CodeZipDevRunnerConfig) {
5548
this.logger = config.logger;
56-
this.run = config.run ?? runCommand;
49+
this.run = config.run ?? runProcess;
5750
this.supervisor = config.supervisor ?? new ProcessSupervisor();
5851
}
5952

6053
public async start(input: StartDevServerInput): Promise<DevServerHandle> {
6154
const directory = join(input.projectRoot, input.runtime.codeLocation);
6255
if (!existsSync(directory)) {
63-
throw new ValidationError(`runtime code directory not found: ${directory}`);
56+
throw new InputValidationError(`runtime code directory not found: ${directory}`);
6457
}
6558

6659
const entrypoint = parseEntrypoint(input.runtime.entrypoint);
@@ -89,9 +82,8 @@ export class CodeZipDevRunner implements DevRunner {
8982
private async ensureNodeModules(directory: string, input: StartDevServerInput): Promise<void> {
9083
if (existsSync(join(directory, "node_modules"))) return;
9184

92-
const packageManager = nodePackageManager(directory);
93-
input.onLog("system", `Installing Node dependencies with ${packageManager}...`);
94-
await this.run([windowsExecutable(packageManager), "install"], {
85+
input.onLog("system", "Installing Node dependencies with npm...");
86+
await this.run([windowsExecutable("npm"), "install"], {
9587
cwd: directory,
9688
onOutput: (chunk) => this.logger.debug(chunk.trim()),
9789
});

src/core/dev/index.ts

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,3 @@
1-
export {
2-
CodeZipDevRunner,
3-
nodePackageManager,
4-
parseEntrypoint,
5-
serverCommand,
6-
type Entrypoint,
7-
} from "./codezip";
1+
export { CodeZipDevRunner, parseEntrypoint, serverCommand, type Entrypoint } from "./codezip";
82
export { findAvailablePort } from "./port";
93
export { ProcessSupervisor, windowsExecutable, type ProcessCommand } from "./process";
10-
export { runCommand, type CommandRunner } from "./run";

src/core/dev/port.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,16 @@ import { createServer } from "node:net";
33
/** How many ports above the requested one to try before giving up. */
44
const MAX_PORT_ATTEMPTS = 100;
55

6-
/** Returns true if `port` can be bound on the loopback interface. */
6+
/** True if `port` is free on loopback. Only EADDRINUSE counts as taken;
7+
* other errors (e.g. EACCES) reject so the caller isn't misled into
8+
* walking past a real failure. */
79
function portFree(port: number): Promise<boolean> {
8-
return new Promise((resolve) => {
10+
return new Promise((resolve, reject) => {
911
const probe = createServer();
10-
probe.once("error", () => resolve(false));
12+
probe.once("error", (error: NodeJS.ErrnoException) => {
13+
if (error.code === "EADDRINUSE") resolve(false);
14+
else reject(error);
15+
});
1116
probe.once("listening", () => probe.close(() => resolve(true)));
1217
probe.listen(port, "127.0.0.1");
1318
});

src/core/dev/process.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ export class ProcessSupervisor {
4040
};
4141
private readonly reapAndExit = (signal: NodeJS.Signals) => {
4242
this.reap();
43-
// Re-deliver the default outcome (exit code 128 + signal number).
43+
// Exit as if unhandled: POSIX convention is 128 + signal number.
4444
process.exit(128 + (signal === "SIGTERM" ? 15 : 1));
4545
};
4646

@@ -91,9 +91,10 @@ export class ProcessSupervisor {
9191
}
9292

9393
private watch(child: ChildProcess): void {
94-
// One set of CLI-exit reapers for all children, attached only while any
95-
// are alive: 'exit' covers normal exit and uncaught exceptions, the
96-
// signal handlers cover terminations that never fire 'exit'.
94+
// Attach the reapers once, on the first live child: the 'exit' handler
95+
// catches normal exits and uncaught exceptions, while the signal handlers
96+
// catch the terminations that never fire 'exit' (see REAP_SIGNALS). Both
97+
// exist so no dev server outlives the CLI and squats on its port.
9798
if (this.children.size === 0) {
9899
process.once("exit", this.reap);
99100
for (const signal of REAP_SIGNALS) process.once(signal, this.reapAndExit);

src/core/dev/run.ts

Lines changed: 0 additions & 38 deletions
This file was deleted.

0 commit comments

Comments
 (0)