assign-ports: warn on malformed explicit port env vars (follow-up to #242)

packages/shaka-shared/src/__tests__/assign-ports.test.ts

@@ -76,6 +76,59 @@ describe('assignPortsAutomatically', () => {
     expect(fs.existsSync(settingsPath)).toBe(false);
   });
 
+  it('warns when an explicit port var is present but not a valid integer, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '40x0', SHAKAPERF_EXPERIMENT_PORT: '4001' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('not a valid port'));
+    warn.mockRestore();
+  });
+
+  it('warns when an explicit port exceeds the TCP range, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '99999', SHAKAPERF_EXPERIMENT_PORT: '4001' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('not a valid port'));
+    warn.mockRestore();
+  });
+
+  it('warns about a privileged explicit port but still uses (and does not persist) the pair', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '80', SHAKAPERF_EXPERIMENT_PORT: '81' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 80, experiment: 81 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('privileged'));
+    expect(fs.existsSync(settingsPath)).toBe(false);
+    warn.mockRestore();
+  });
+
+  it('warns when only one explicit port var is set, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = { ...baseDeps(), env: { SHAKAPERF_CONTROL_PORT: '4000' } };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('must be set to valid ports'));
+    warn.mockRestore();
+  });
+
+  it('treats blank/absent explicit port vars as unset without warning', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '  ', SHAKAPERF_EXPERIMENT_PORT: '' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).not.toHaveBeenCalled();
+    warn.mockRestore();
+  });
+
   it('derives the pair from SHAKAPERF_BASE_PORT and does not persist it', () => {
     const deps: AssignPortsDeps = { ...baseDeps(), env: { SHAKAPERF_BASE_PORT: '4200' } };
     expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 4200, experiment: 4201 });

packages/shaka-shared/src/assign-ports.ts

@@ -123,12 +123,52 @@ function lsofPortInUse(port: number): boolean {
   }
 }
 
+/**
+ * Parse one explicit-port env var (`SHAKAPERF_CONTROL_PORT` /
+ * `SHAKAPERF_EXPERIMENT_PORT`). Returns the port when present and valid; null
+ * when absent or blank (silently). Validation matches `readBasePortOverride`: a
+ * positive integer within `1..MAX_PORT` (a present-but-invalid value warns and
+ * returns null), plus a privileged-range (`<= PRIVILEGED_PORT_MAX`) advisory that
+ * warns but still returns the port. Keeps a typo'd value from being swallowed
+ * silently.
+ */
+function parseExplicitPort(env: NodeJS.ProcessEnv, varName: string): number | null {
+  const raw = env[varName];
+  if (raw == null) return null;
+  const stripped = raw.trim();
+  if (stripped === '') return null;
+  const port = Number(stripped);
+  if (!/^\d+$/.test(stripped) || port < 1 || port > MAX_PORT) {
+    console.warn(`assignPortsAutomatically: ${varName}="${raw}" is not a valid port (1..${MAX_PORT}); ignoring.`);
+    return null;
+  }
+  if (port <= PRIVILEGED_PORT_MAX) {
+    console.warn(
+      `assignPortsAutomatically: ${varName}="${raw}" is in the privileged range ` +
+        `(1..${PRIVILEGED_PORT_MAX}); binding will fail without root.`,
+    );
+  }
+  return port;
+}
+
 function readEnvOverride(env: NodeJS.ProcessEnv): AssignedPorts | null {
-  const control = Number(env.SHAKAPERF_CONTROL_PORT);
-  const experiment = Number(env.SHAKAPERF_EXPERIMENT_PORT);
-  if (Number.isInteger(control) && control > 0 && Number.isInteger(experiment) && experiment > 0) {
+  const control = parseExplicitPort(env, 'SHAKAPERF_CONTROL_PORT');
+  const experiment = parseExplicitPort(env, 'SHAKAPERF_EXPERIMENT_PORT');
+  if (control != null && experiment != null) {
     return { control, experiment };
   }
+  // Both vars are required to pin an explicit pair. If either was provided (set
+  // and non-blank) but the pair didn't resolve, that's almost always a mistake —
+  // warn that the whole override was dropped, not just the bad half. (A
+  // present-but-invalid value has already warned in parseExplicitPort.)
+  const isPresent = (value?: string): boolean => value != null && value.trim() !== '';
+  if (isPresent(env.SHAKAPERF_CONTROL_PORT) || isPresent(env.SHAKAPERF_EXPERIMENT_PORT)) {
+    console.warn(
+      'assignPortsAutomatically: both SHAKAPERF_CONTROL_PORT and ' +
+        'SHAKAPERF_EXPERIMENT_PORT must be set to valid ports to pin an explicit ' +
+        'pair; falling through to automatic assignment.',
+    );
+  }
   return null;
 }