diff --git a/packages/shaka-shared/src/__tests__/assign-ports.test.ts b/packages/shaka-shared/src/__tests__/assign-ports.test.ts index 8de1cbe..b43ef1d 100644 --- a/packages/shaka-shared/src/__tests__/assign-ports.test.ts +++ b/packages/shaka-shared/src/__tests__/assign-ports.test.ts @@ -31,6 +31,7 @@ describe('assignPortsAutomatically', () => { }); afterEach(() => { + jest.restoreAllMocks(); fs.rmSync(dir, { recursive: true, force: true }); }); @@ -76,6 +77,70 @@ 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')); + expect(warn).toHaveBeenCalledTimes(1); + 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')); + expect(warn).toHaveBeenCalledTimes(1); + 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('does not add the generic pair warning when the lone explicit var is malformed', () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const deps: AssignPortsDeps = { ...baseDeps(), env: { SHAKAPERF_EXPERIMENT_PORT: '40x0' } }; + expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 }); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('not a valid port')); + expect(warn).toHaveBeenCalledTimes(1); + 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 }); diff --git a/packages/shaka-shared/src/assign-ports.ts b/packages/shaka-shared/src/assign-ports.ts index 228bf77..4d8e936 100644 --- a/packages/shaka-shared/src/assign-ports.ts +++ b/packages/shaka-shared/src/assign-ports.ts @@ -123,12 +123,53 @@ 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 mirrors `readBasePortOverride` + * for malformed and privileged values, but explicit ports may use the full + * `1..MAX_PORT` range because no derived experiment offset is added. 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 one side parsed as valid + // and the other side was absent/blank, warn that the whole override was + // dropped. Malformed non-blank values already emitted a specific warning in + // parseExplicitPort, so avoid adding a second generic message. + const isPresent = (value?: string): boolean => value != null && value.trim() !== ''; + const controlPresent = isPresent(env.SHAKAPERF_CONTROL_PORT); + const experimentPresent = isPresent(env.SHAKAPERF_EXPERIMENT_PORT); + if ((control != null && !experimentPresent) || (experiment != null && !controlPresent)) { + 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; }