diff --git a/src/oclif/commands/webui.ts b/src/oclif/commands/webui.ts index 1beb27df7..38fd7d0ed 100644 --- a/src/oclif/commands/webui.ts +++ b/src/oclif/commands/webui.ts @@ -1,6 +1,11 @@ import {Command, Flags} from '@oclif/core' import open from 'open' +import { + WebuiEvents, + type WebuiGetPortResponse, + type WebuiSetPortResponse, +} from '../../shared/transport/events/webui-events.js' import {formatConnectionError, withDaemonRetry} from '../lib/daemon-client.js' export default class Webui extends Command { @@ -16,38 +21,58 @@ export default class Webui extends Command { public async run(): Promise { const {flags} = await this.parse(Webui) - let webuiPort: number + const webuiPort = flags.port ? await this.resolveSetPort(flags.port) : await this.resolveGetPort() + const url = `http://localhost:${webuiPort}` + this.log(`ByteRover Web UI: ${url}`) + + await open(url).catch(() => { + this.log('Could not open browser automatically. Open the URL above manually.') + }) + } + private async resolveGetPort(): Promise { + let result: WebuiGetPortResponse try { - // If --port is provided, tell the daemon to switch to that port and persist it - if (flags.port) { - const result = await withDaemonRetry( - async (client) => - client.requestWithAck<{port: number; success: boolean}>('webui:setPort', {port: flags.port}), - {projectPath: process.cwd()}, - ) - webuiPort = result.port - } else { - const result = await withDaemonRetry( - async (client) => client.requestWithAck<{port?: number}>('webui:getPort'), - {projectPath: process.cwd()}, - ) - - if (!result.port) { - this.error('Failed to get web UI port. Use `brv restart` to restart the daemon and try again') - } - - webuiPort = result.port + result = await withDaemonRetry( + async (client) => client.requestWithAck(WebuiEvents.GET_PORT), + {projectPath: process.cwd()}, + ) + } catch (error) { + return this.error(formatConnectionError(error)) + } + + if (result.status === 'ok') { + if (result.requestedPort !== undefined && result.requestedPort !== result.port) { + this.log(`Port ${result.requestedPort} was in use — using port ${result.port} instead.`) } + + return result.port + } + + if (result.status === 'port_in_use') { + return this.error( + `Web UI port ${result.conflictPort} is already in use. Run \`brv webui --port \` to choose a different port.`, + ) + } + + return this.error('Web UI did not start. Run `brv restart` and try again.') + } + + private async resolveSetPort(port: number): Promise { + let result: WebuiSetPortResponse + try { + result = await withDaemonRetry( + async (client) => client.requestWithAck(WebuiEvents.SET_PORT, {port}), + {projectPath: process.cwd()}, + ) } catch (error) { - this.error(formatConnectionError(error)) + return this.error(formatConnectionError(error)) } - const url = `http://localhost:${webuiPort}` - this.log(`ByteRover Web UI: ${url}`) + if (result.status === 'ok') return result.port - await open(url).catch(() => { - this.log('Could not open browser automatically. Open the URL above manually.') - }) + return this.error( + `Web UI port ${result.conflictPort} is already in use. Run \`brv webui --port \` to choose a different port.`, + ) } } diff --git a/src/server/constants.ts b/src/server/constants.ts index 1e091be8e..be4cd5081 100644 --- a/src/server/constants.ts +++ b/src/server/constants.ts @@ -61,6 +61,7 @@ export const PORT_BATCH_SIZE = 20 export const PORT_MAX_ATTEMPTS = 5 // Web UI (stable port, separate from dynamic transport port) export const WEBUI_DEFAULT_PORT = 7700 +export const WEBUI_MAX_FALLBACK_ATTEMPTS = 10 export const WEBUI_STATE_FILE = 'webui.json' // Heartbeat export const HEARTBEAT_FILE = 'heartbeat' diff --git a/src/server/core/domain/errors/webui-error.ts b/src/server/core/domain/errors/webui-error.ts new file mode 100644 index 000000000..6ed2c5560 --- /dev/null +++ b/src/server/core/domain/errors/webui-error.ts @@ -0,0 +1,23 @@ +export class WebUiError extends Error { + public constructor(message: string) { + super(message) + this.name = 'WebUiError' + } +} + +export class WebUiPortInUseError extends WebUiError { + public readonly port: number + + public constructor(port: number) { + super(`Web UI port ${port} is already in use`) + this.name = 'WebUiPortInUseError' + this.port = port + } +} + +export class WebUiServerAlreadyRunningError extends WebUiError { + public constructor() { + super('Web UI server is already running') + this.name = 'WebUiServerAlreadyRunningError' + } +} diff --git a/src/server/infra/daemon/brv-server.ts b/src/server/infra/daemon/brv-server.ts index ba8fc746e..3375aee70 100644 --- a/src/server/infra/daemon/brv-server.ts +++ b/src/server/infra/daemon/brv-server.ts @@ -33,6 +33,12 @@ import type {BrvConfig} from '../../core/domain/entities/brv-config.js' import {ReviewEvents} from '../../../shared/transport/events/review-events.js' import {TaskEvents, type TaskHeartbeatEvent} from '../../../shared/transport/events/task-events.js' +import { + WebuiEvents, + type WebuiGetPortResponse, + type WebuiSetPortRequest, + type WebuiSetPortResponse, +} from '../../../shared/transport/events/webui-events.js' import { AGENT_IDLE_CHECK_INTERVAL_MS, AGENT_IDLE_TIMEOUT_MS, @@ -40,7 +46,9 @@ import { HEARTBEAT_FILE, TASK_HEARTBEAT_INTERVAL_MS, WEBUI_DEFAULT_PORT, + WEBUI_MAX_FALLBACK_ATTEMPTS, } from '../../constants.js' +import {WebUiPortInUseError} from '../../core/domain/errors/webui-error.js' import { type ProviderConfigResponse, type TaskCurateResultEvent, @@ -92,6 +100,7 @@ import {IdleTimeoutPolicy} from './idle-timeout-policy.js' import {selectDaemonPort} from './port-selector.js' import {bootstrapSettings} from './settings-bootstrap.js' import {ShutdownHandler} from './shutdown-handler.js' +import {startWebUiWithFallback} from './start-webui-with-fallback.js' function log(msg: string): void { processLog(`[Daemon] ${msg}`) @@ -196,6 +205,8 @@ async function main(): Promise { let authStateStore: AuthStateStore | undefined let agentPool: AgentPool | undefined let webuiServer: undefined | WebUiServer + let webuiBootFailure: undefined | {conflictPort: number; status: 'port_in_use'} + let webuiRequestedPort: number | undefined try { // 4a. Construct transport server. start() is deferred to step 11 so all handlers register before sockets connect. @@ -207,12 +218,19 @@ async function main(): Promise { const webuiDistDir = join(projectRoot, 'dist', 'webui') // Port priority: env var > persisted preference > default const webuiPortEnv = process.env.BRV_WEBUI_PORT - const webuiPort = webuiPortEnv - ? Number.parseInt(webuiPortEnv, 10) - : (readWebuiPreferredPort() ?? WEBUI_DEFAULT_PORT) + const explicitWebuiPort = webuiPortEnv ? Number.parseInt(webuiPortEnv, 10) : readWebuiPreferredPort() + const webuiPreferredPort = explicitWebuiPort ?? WEBUI_DEFAULT_PORT + // Explicit user intent (env/persisted) is honored strictly. The default + // port is the only one that may fall back to neighbors when busy. + const webuiMaxAttempts = explicitWebuiPort === undefined ? WEBUI_MAX_FALLBACK_ATTEMPTS : 1 const webuiApp = createWebUiMiddleware({ - getConfig: () => ({daemonPort: port, port: webuiPort, projectCwd: process.cwd(), version}), + getConfig: () => ({ + daemonPort: port, + port: webuiServer?.getPort() ?? webuiPreferredPort, + projectCwd: process.cwd(), + version, + }), webuiDistDir, }) @@ -220,16 +238,31 @@ async function main(): Promise { app.use(webuiApp) webuiServer = new WebUiServer(app) - try { - await webuiServer.start(webuiPort) - writeWebuiState(webuiPort) - log(`Web UI server started on port ${webuiPort}`) - } catch (webuiError) { - log( - `Web UI port ${webuiPort} is already in use. Web UI will not be available. Set BRV_WEBUI_PORT= to use a different port.`, - ) - log(`Web UI start error: ${webuiError instanceof Error ? webuiError.message : String(webuiError)}`) + const webuiOutcome = await startWebUiWithFallback(webuiServer, webuiPreferredPort, webuiMaxAttempts) + + if (webuiOutcome.status === 'ok') { + writeWebuiState(webuiOutcome.actualPort) + if (webuiOutcome.actualPort === webuiOutcome.requestedPort) { + log(`Web UI server started on port ${webuiOutcome.actualPort}`) + } else { + webuiRequestedPort = webuiOutcome.requestedPort + log(`Web UI port ${webuiOutcome.requestedPort} was in use — started on ${webuiOutcome.actualPort} instead`) + } + } else { webuiServer = undefined + if (webuiOutcome.error instanceof WebUiPortInUseError) { + webuiBootFailure = {conflictPort: webuiPreferredPort, status: 'port_in_use'} + const rangeEnd = webuiPreferredPort + webuiMaxAttempts - 1 + log( + webuiMaxAttempts > 1 + ? `Web UI ports ${webuiPreferredPort}-${rangeEnd} are all in use — Web UI unavailable` + : `Web UI port ${webuiPreferredPort} is already in use — Web UI unavailable`, + ) + } else { + log( + `Web UI start error: ${webuiOutcome.error instanceof Error ? webuiOutcome.error.message : String(webuiOutcome.error)}`, + ) + } } // 5. Start heartbeat writer. Must run before transport.start(): pollForDaemon SIGTERMs daemons with stale heartbeat. @@ -565,12 +598,19 @@ async function main(): Promise { }) // Web UI port endpoint — used by `brv webui` to discover the stable port - transportServer.onRequest('webui:getPort', () => ({ - port: webuiServer?.getPort(), - })) + transportServer.onRequest(WebuiEvents.GET_PORT, () => { + const activePort = webuiServer?.getPort() + if (activePort !== undefined) { + return webuiRequestedPort !== undefined && webuiRequestedPort !== activePort + ? {port: activePort, requestedPort: webuiRequestedPort, status: 'ok'} + : {port: activePort, status: 'ok'} + } + + return webuiBootFailure ?? {status: 'not_started'} + }) // Web UI set port — restarts webui server on new port and persists preference - transportServer.onRequest<{port: number}, {port: number; success: boolean}>('webui:setPort', async (data) => { + transportServer.onRequest(WebuiEvents.SET_PORT, async (data) => { const newPort = data.port // Stop existing webui server if running @@ -589,12 +629,27 @@ async function main(): Promise { // Start on new port webuiServer = new WebUiServer(newApp) - await webuiServer.start(newPort) + try { + await webuiServer.start(newPort) + } catch (error) { + webuiServer = undefined + if (error instanceof WebUiPortInUseError) { + webuiBootFailure = {conflictPort: error.port, status: 'port_in_use'} + log(`Web UI port ${error.port} is already in use — Web UI unavailable`) + return {conflictPort: error.port, status: 'port_in_use'} + } + + webuiBootFailure = undefined + throw error + } + + webuiBootFailure = undefined + webuiRequestedPort = undefined writeWebuiState(newPort) writeWebuiPreferredPort(newPort) log(`Web UI server restarted on port ${newPort} (persisted)`) - return {port: newPort, success: true} + return {port: newPort, status: 'ok'} }) // Debug endpoint — exposes daemon internal state for `brv debug` command diff --git a/src/server/infra/daemon/start-webui-with-fallback.ts b/src/server/infra/daemon/start-webui-with-fallback.ts new file mode 100644 index 000000000..83614ebf1 --- /dev/null +++ b/src/server/infra/daemon/start-webui-with-fallback.ts @@ -0,0 +1,31 @@ +import {WebUiPortInUseError} from '../../core/domain/errors/webui-error.js' + +export interface WebUiStarter { + start(port: number): Promise +} + +export type StartWebUiOutcome = + | {actualPort: number; requestedPort: number; status: 'ok'} + | {error: unknown; status: 'error'} + +export async function startWebUiWithFallback( + server: WebUiStarter, + preferredPort: number, + maxAttempts: number, +): Promise { + let lastError: unknown + for (let offset = 0; offset < maxAttempts; offset++) { + const attemptPort = preferredPort + offset + try { + // eslint-disable-next-line no-await-in-loop -- intentional sequential fallback + await server.start(attemptPort) + return {actualPort: attemptPort, requestedPort: preferredPort, status: 'ok'} + } catch (error) { + lastError = error + if (error instanceof WebUiPortInUseError) continue + break + } + } + + return {error: lastError, status: 'error'} +} diff --git a/src/server/infra/webui/webui-server.ts b/src/server/infra/webui/webui-server.ts index ee793e8da..3175d15d1 100644 --- a/src/server/infra/webui/webui-server.ts +++ b/src/server/infra/webui/webui-server.ts @@ -3,6 +3,7 @@ import type {Express} from 'express' import {createServer, type Server as HttpServer} from 'node:http' import {TRANSPORT_HOST} from '../../constants.js' +import {WebUiPortInUseError, WebUiServerAlreadyRunningError} from '../../core/domain/errors/webui-error.js' /** * Standalone HTTP server for the web UI. @@ -28,21 +29,21 @@ export class WebUiServer { async start(port: number): Promise { if (this.running) { - throw new Error('Web UI server is already running') + throw new WebUiServerAlreadyRunningError() } return new Promise((resolve, reject) => { this.httpServer = createServer(this.app) + let resolved = false this.httpServer.on('error', (err: NodeJS.ErrnoException) => { - if (err.code === 'EADDRINUSE') { - reject(new Error(`Web UI port ${port} is already in use`)) - } else { - reject(err) - } + if (resolved) return + this.httpServer = undefined + reject(err.code === 'EADDRINUSE' ? new WebUiPortInUseError(port) : err) }) this.httpServer.listen(port, TRANSPORT_HOST, () => { + resolved = true const addr = this.httpServer?.address() this.port = typeof addr === 'object' && addr !== null ? addr.port : port this.running = true diff --git a/src/shared/transport/events/index.ts b/src/shared/transport/events/index.ts index 8abba92ae..6d3ea122e 100644 --- a/src/shared/transport/events/index.ts +++ b/src/shared/transport/events/index.ts @@ -29,6 +29,7 @@ export * from './status-events.js' export * from './task-events.js' export * from './team-events.js' export * from './vc-events.js' +export * from './webui-events.js' export * from './worktree-events.js' // Utility exports @@ -59,6 +60,7 @@ import {StatusEvents} from './status-events.js' import {TaskEvents} from './task-events.js' import {TeamEvents} from './team-events.js' import {VcEvents} from './vc-events.js' +import {WebuiEvents} from './webui-events.js' import {WorktreeEvents} from './worktree-events.js' /** @@ -93,6 +95,7 @@ export const AllEventGroups = [ TaskEvents, TeamEvents, VcEvents, + WebuiEvents, WorktreeEvents, ] as const diff --git a/src/shared/transport/events/webui-events.ts b/src/shared/transport/events/webui-events.ts new file mode 100644 index 000000000..27893f6fa --- /dev/null +++ b/src/shared/transport/events/webui-events.ts @@ -0,0 +1,16 @@ +export const WebuiEvents = { + GET_PORT: 'webui:getPort', + SET_PORT: 'webui:setPort', +} as const + +type WebuiGetPortOkResponse = {port: number; requestedPort?: number; status: 'ok'} +type WebuiPortInUseResponse = {conflictPort: number; status: 'port_in_use'} +type WebuiSetPortOkResponse = {port: number; status: 'ok'} + +export type WebuiGetPortResponse = WebuiGetPortOkResponse | WebuiPortInUseResponse | {status: 'not_started'} + +export interface WebuiSetPortRequest { + port: number +} + +export type WebuiSetPortResponse = WebuiPortInUseResponse | WebuiSetPortOkResponse diff --git a/test/unit/core/domain/errors/webui-error.test.ts b/test/unit/core/domain/errors/webui-error.test.ts new file mode 100644 index 000000000..d91dd054e --- /dev/null +++ b/test/unit/core/domain/errors/webui-error.test.ts @@ -0,0 +1,33 @@ +import {expect} from 'chai' + +import { + WebUiError, + WebUiPortInUseError, + WebUiServerAlreadyRunningError, +} from '../../../../../src/server/core/domain/errors/webui-error.js' + +describe('webui-error', () => { + describe('WebUiPortInUseError', () => { + it('exposes the conflicting port and a descriptive message', () => { + const error = new WebUiPortInUseError(7700) + + expect(error).to.be.an.instanceOf(WebUiError) + expect(error).to.be.an.instanceOf(Error) + expect(error.name).to.equal('WebUiPortInUseError') + expect(error.port).to.equal(7700) + expect(error.message).to.include('7700') + expect(error.message).to.include('in use') + }) + }) + + describe('WebUiServerAlreadyRunningError', () => { + it('identifies itself by name and extends WebUiError', () => { + const error = new WebUiServerAlreadyRunningError() + + expect(error).to.be.an.instanceOf(WebUiError) + expect(error).to.be.an.instanceOf(Error) + expect(error.name).to.equal('WebUiServerAlreadyRunningError') + expect(error.message).to.include('already running') + }) + }) +}) diff --git a/test/unit/infra/daemon/start-webui-with-fallback.test.ts b/test/unit/infra/daemon/start-webui-with-fallback.test.ts new file mode 100644 index 000000000..cd0ffab1f --- /dev/null +++ b/test/unit/infra/daemon/start-webui-with-fallback.test.ts @@ -0,0 +1,78 @@ +import {expect} from 'chai' +import express from 'express' +import {restore, type SinonStub, stub} from 'sinon' + +import {WebUiPortInUseError} from '../../../../src/server/core/domain/errors/webui-error.js' +import {startWebUiWithFallback} from '../../../../src/server/infra/daemon/start-webui-with-fallback.js' +import {WebUiServer} from '../../../../src/server/infra/webui/webui-server.js' + +describe('startWebUiWithFallback', () => { + let server: WebUiServer + let startStub: SinonStub + + beforeEach(() => { + server = new WebUiServer(express()) + startStub = stub(server, 'start') + }) + + afterEach(() => { + restore() + }) + + it('returns the preferred port on first-attempt success', async () => { + startStub.resolves() + + const outcome = await startWebUiWithFallback(server, 7700, 10) + + expect(outcome).to.deep.equal({actualPort: 7700, requestedPort: 7700, status: 'ok'}) + expect(startStub.calledOnceWithExactly(7700)).to.be.true + }) + + it('falls back to the next port when the preferred port is in use', async () => { + startStub.onFirstCall().rejects(new WebUiPortInUseError(7700)) + startStub.onSecondCall().resolves() + + const outcome = await startWebUiWithFallback(server, 7700, 10) + + expect(outcome).to.deep.equal({actualPort: 7701, requestedPort: 7700, status: 'ok'}) + expect(startStub.callCount).to.equal(2) + expect(startStub.firstCall.args[0]).to.equal(7700) + expect(startStub.secondCall.args[0]).to.equal(7701) + }) + + it('returns the last error when every attempt fails with EADDRINUSE', async () => { + startStub.rejects(new WebUiPortInUseError(7700)) + + const outcome = await startWebUiWithFallback(server, 7700, 3) + + expect(outcome.status).to.equal('error') + if (outcome.status === 'error') { + expect(outcome.error).to.be.an.instanceOf(WebUiPortInUseError) + } + + expect(startStub.callCount).to.equal(3) + }) + + it('treats maxAttempts=1 as strict mode (no fallback)', async () => { + startStub.rejects(new WebUiPortInUseError(9090)) + + const outcome = await startWebUiWithFallback(server, 9090, 1) + + expect(outcome.status).to.equal('error') + expect(startStub.calledOnceWithExactly(9090)).to.be.true + }) + + it('breaks immediately on non-EADDRINUSE errors', async () => { + const genericError = new Error('permission denied') + startStub.rejects(genericError) + + const outcome = await startWebUiWithFallback(server, 7700, 10) + + expect(outcome.status).to.equal('error') + if (outcome.status === 'error') { + expect(outcome.error).to.equal(genericError) + } + + expect(startStub.calledOnce).to.be.true + }) +}) diff --git a/test/unit/infra/webui/webui-server.test.ts b/test/unit/infra/webui/webui-server.test.ts index 0df3d3aa1..80ff24822 100644 --- a/test/unit/infra/webui/webui-server.test.ts +++ b/test/unit/infra/webui/webui-server.test.ts @@ -2,6 +2,7 @@ import {expect} from 'chai' import express from 'express' import {createServer} from 'node:http' +import {WebUiPortInUseError, WebUiServerAlreadyRunningError} from '../../../../src/server/core/domain/errors/webui-error.js' import {WebUiServer} from '../../../../src/server/infra/webui/webui-server.js' describe('WebUiServer', () => { @@ -34,7 +35,7 @@ describe('WebUiServer', () => { expect(server.getPort()).to.be.undefined }) - it('should reject with error when port is in use', async () => { + it('should reject with WebUiPortInUseError when port is in use', async () => { // Occupy a port first const blockingServer = createServer() const occupiedPort = await new Promise((resolve, reject) => { @@ -53,11 +54,13 @@ describe('WebUiServer', () => { await server.start(occupiedPort) expect.fail('Expected start to reject') } catch (error) { - expect(error).to.be.an.instanceOf(Error) + expect(error).to.be.an.instanceOf(WebUiPortInUseError) + expect((error as WebUiPortInUseError).port).to.equal(occupiedPort) expect((error as Error).message).to.include('in use') } expect(server.isRunning()).to.be.false + expect(server.getPort()).to.be.undefined } finally { blockingServer.close() } @@ -70,8 +73,7 @@ describe('WebUiServer', () => { await server.start(0) expect.fail('Expected start to reject') } catch (error) { - expect(error).to.be.an.instanceOf(Error) - expect((error as Error).message).to.include('already running') + expect(error).to.be.an.instanceOf(WebUiServerAlreadyRunningError) } }) @@ -79,4 +81,21 @@ describe('WebUiServer', () => { server = new WebUiServer(express()) await server.stop() // should not throw }) + + it('should ignore post-startup error emissions so stop() still cleans up', async () => { + server = new WebUiServer(express()) + await server.start(0) + const boundPort = server.getPort() + expect(boundPort).to.be.a('number') + + const internal = server as unknown as {httpServer?: {emit(event: string, err: Error): boolean}} + internal.httpServer?.emit('error', Object.assign(new Error('ECONNRESET'), {code: 'ECONNRESET'})) + + expect(server.isRunning()).to.be.true + expect(server.getPort()).to.equal(boundPort) + + await server.stop() + expect(server.isRunning()).to.be.false + expect(server.getPort()).to.be.undefined + }) })