diff --git a/.changeset/proxy-error-visibility.md b/.changeset/proxy-error-visibility.md new file mode 100644 index 00000000000..e054af5ed34 --- /dev/null +++ b/.changeset/proxy-error-visibility.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Surface local proxy errors during `app dev`: unmatched HTTP paths and websocket upgrades now log a warning instead of failing silently, the "Proxy server started" line only prints after the socket actually binds, and a fatal proxy error (failed bind, or a runtime error after bind) tears the dev session down instead of leaving the proxy dead with no further indication. diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index aeb6702a4ea..c16adc26277 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -32,6 +32,7 @@ import {DeveloperPlatformClient} from '../../../utilities/developer-platform-cli import {AppEventWatcher} from '../app-events/app-event-watcher.js' import * as loader from '../../../models/app/loader.js' import {describe, test, expect, beforeEach, vi} from 'vitest' +import {AbortController} from '@shopify/cli-kit/node/abort' import {ensureAuthenticatedAdmin, ensureAuthenticatedStorefront} from '@shopify/cli-kit/node/session' import {Config} from '@oclif/core' import {getEnvironmentVariables} from '@shopify/cli-kit/node/environment' @@ -39,6 +40,9 @@ import {isStorefrontPasswordProtected} from '@shopify/theme' import {fetchTheme} from '@shopify/cli-kit/node/themes/api' import {firstPartyDev} from '@shopify/cli-kit/node/context/local' import {adminFqdn} from '@shopify/cli-kit/node/context/fqdn' +import {Writable} from 'stream' +import net from 'net' +import http from 'http' vi.mock('../../context/identifiers.js') vi.mock('@shopify/cli-kit/node/session.js') @@ -737,3 +741,51 @@ describe('setup-dev-processes', () => { }) }) }) + +describe('startProxyServer', () => { + const sinkStream = () => + new Writable({ + write(_chunk, _encoding, next) { + next() + }, + }) + + test('rejects with a user-friendly EADDRINUSE message when the port is already in use', async () => { + // Hold a port so the proxy bind fails with EADDRINUSE. + const blocker = http.createServer() + await new Promise((resolve) => blocker.listen(0, 'localhost', resolve)) + const port = (blocker.address() as net.AddressInfo).port + + const abortController = new AbortController() + try { + await expect( + startProxyServer( + {abortSignal: abortController.signal, stdout: sinkStream(), stderr: sinkStream()}, + {port, rules: {default: 'http://localhost:1'}}, + ), + ).rejects.toThrow(`Reverse HTTP proxy error - Port ${port} is already in use`) + } finally { + abortController.abort() + await new Promise((resolve) => blocker.close(() => resolve())) + } + }) + + test('stays alive after bind and resolves cleanly when the abort signal fires', async () => { + const abortController = new AbortController() + let resolved = false + const proxyPromise = startProxyServer( + {abortSignal: abortController.signal, stdout: sinkStream(), stderr: sinkStream()}, + {port: 0, rules: {default: 'http://localhost:1'}}, + ).then(() => { + resolved = true + }) + + // Give the proxy enough time to bind and enter its long-running wait. + await new Promise((resolve) => setTimeout(resolve, 50)) + expect(resolved).toBe(false) + + abortController.abort() + await expect(proxyPromise).resolves.toBeUndefined() + expect(resolved).toBe(true) + }) +}) diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts index 0c6ca0b2f6d..46bfa871d29 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts @@ -25,7 +25,8 @@ import {getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' import {isTruthy} from '@shopify/cli-kit/node/context/utilities' import {firstPartyDev} from '@shopify/cli-kit/node/context/local' import {getEnvironmentVariables} from '@shopify/cli-kit/node/environment' -import {outputInfo} from '@shopify/cli-kit/node/output' +import {outputInfo, outputWarn} from '@shopify/cli-kit/node/output' +import {useConcurrentOutputContext} from '@shopify/cli-kit/node/ui/components' import {adminFqdn} from '@shopify/cli-kit/node/context/fqdn' interface ProxyServerProcess extends BaseProcess<{ @@ -295,9 +296,57 @@ export const startProxyServer: DevProcessFunction<{ localhostCert?: LocalhostCert }> = async ({abortSignal, stdout}, {port, rules, localhostCert}) => { const {server} = await getProxyingWebServer(rules, abortSignal, localhostCert, stdout) + + // `server.listen` is event-based and returns the Server synchronously, so awaiting it + // does not actually wait for the socket to bind. Wrap it in a promise that resolves on + // 'listening' and rejects on 'error' (e.g. EADDRINUSE) so a failed bind surfaces to the + // dev runner instead of crashing Node via an uncaught 'error' event. + await new Promise((resolve, reject) => { + const onError = (err: NodeJS.ErrnoException) => { + server.off('listening', onListening) + reject(translateBindError(err, port)) + } + const onListening = () => { + server.off('error', onError) + resolve() + } + server.once('error', onError) + server.once('listening', onListening) + server.listen(port, 'localhost') + }) + outputInfo( `Proxy server started on port ${port} ${localhostCert ? `with certificate ${localhostCert.certPath}` : ''}`, stdout, ) - await server.listen(port, 'localhost') + + // Stay alive for the lifetime of the dev session. Resolve cleanly when the abort signal + // fires; reject if the server emits a post-bind 'error' so a dead proxy tears down the + // dev session instead of leaving the user with one warning line and silently broken + // request forwarding for the rest of the session. + await new Promise((resolve, reject) => { + const cleanup = () => { + abortSignal.removeEventListener('abort', onAbort) + server.off('error', onError) + } + const onAbort = () => { + cleanup() + resolve() + } + const onError = (err: Error) => { + cleanup() + useConcurrentOutputContext({outputPrefix: 'proxy', stripAnsi: false}, () => { + outputWarn(`Reverse HTTP proxy error - Server stopped: ${err.message}`, stdout) + }) + reject(err) + } + server.on('error', onError) + abortSignal.addEventListener('abort', onAbort, {once: true}) + }) +} + +function translateBindError(err: NodeJS.ErrnoException, port: number): Error { + if (err.code === 'EADDRINUSE') return new Error(`Reverse HTTP proxy error - Port ${port} is already in use`) + if (err.code === 'EACCES') return new Error(`Reverse HTTP proxy error - Permission denied binding port ${port}`) + return new Error(`Reverse HTTP proxy error - Couldn't start on port ${port}: ${err.message}`) } diff --git a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts index 5b7f9a64db5..5b111fddd78 100644 --- a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts +++ b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts @@ -69,6 +69,56 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { }) }) +describe.sequential('http-reverse-proxy without a default rule', () => { + test('returns 500 for unmatched HTTP paths', {retry: 2}, async () => { + const abortController = new AbortController() + const targetServer = http.createServer((_req, res) => { + res.writeHead(200) + res.end('ok') + }) + await new Promise((resolve) => targetServer.listen(0, 'localhost', resolve)) + const targetPort = (targetServer.address() as net.AddressInfo).port + + const {server} = await getProxyingWebServer({'/known': `http://localhost:${targetPort}`}, abortController.signal) + await new Promise((resolve) => server.listen(0, 'localhost', resolve)) + const proxyPort = (server.address() as net.AddressInfo).port + + try { + const response = await fetch(`http://localhost:${proxyPort}/unknown`, { + agent: new http.Agent({keepAlive: false}), + }) + expect(response.status).toBe(500) + await expect(response.text()).resolves.toContain('Invalid path') + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + targetServer.closeAllConnections() + await new Promise((resolve) => targetServer.close(() => resolve())) + } + }) + + test('destroys websocket connections that do not match any rule', {retry: 2}, async () => { + const abortController = new AbortController() + const {server} = await getProxyingWebServer({'/known': 'http://localhost:1'}, abortController.signal) + await new Promise((resolve) => server.listen(0, 'localhost', resolve)) + const proxyPort = (server.address() as net.AddressInfo).port + + try { + await new Promise((resolve, reject) => { + const ws = new WebSocket(`ws://localhost:${proxyPort}/unmatched`, { + agent: new http.Agent({keepAlive: false}), + }) + ws.on('open', () => reject(new Error('connection should not have opened'))) + ws.on('error', () => resolve()) + ws.on('unexpected-response', () => resolve()) + }) + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } + }) +}) + function getTestReverseProxy(protocol: 'http' | 'https') { return test.extend<{ setup: { diff --git a/packages/app/src/cli/utilities/app/http-reverse-proxy.ts b/packages/app/src/cli/utilities/app/http-reverse-proxy.ts index 9304bab8e84..46d63c942b5 100644 --- a/packages/app/src/cli/utilities/app/http-reverse-proxy.ts +++ b/packages/app/src/cli/utilities/app/http-reverse-proxy.ts @@ -58,6 +58,9 @@ function getProxyServerWebsocketUpgradeListener( }) }) } + useConcurrentOutputContext({outputPrefix: 'proxy', stripAnsi: false}, () => { + outputWarn(`Reverse HTTP proxy error - Invalid websocket path: ${req.url ?? ''}`, stdout) + }) socket.destroy() } } @@ -80,6 +83,9 @@ function getProxyServerRequestListener( }) } + useConcurrentOutputContext({outputPrefix: 'proxy', stripAnsi: false}, () => { + outputWarn(`Reverse HTTP proxy error - Invalid path: ${req.url ?? ''}`, stdout) + }) outputDebug(outputContent` Reverse HTTP proxy error - Invalid path: ${req.url ?? ''} These are the allowed paths: