-
Notifications
You must be signed in to change notification settings - Fork 106
fix: nitro app workaround #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 32 commits
aa41312
efc7f00
dedfe1a
b519d38
f74a8cf
e628752
8adf7c4
7393a48
f0426d2
981a796
a6c8b60
ee26a58
83868ed
1826088
0f8f6ad
48756f0
0bf618f
59beb55
faa91cb
06bb751
5d7b1c8
506b994
3d540e9
275d0d4
5ff5c74
7a036f2
417274f
7ce66e5
1088e9d
23b6b76
f265a06
cbd26a6
311da18
4f1d678
29e194f
e06f4cd
ddf967b
3a46245
1408545
03f322a
b671e6b
4e4e8e4
d354b78
fa47c36
76380f4
fbde6dd
e7d1a13
09913e4
f1fa701
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,80 +1,165 @@ | ||
| import http from 'node:http'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import type { AddressInfo } from 'node:net'; | ||
| import { afterEach, describe, expect, it } from 'vitest'; | ||
| import { getPort } from './get-port'; | ||
|
|
||
| describe('getPort', () => { | ||
| it('should return undefined or a positive number', async () => { | ||
| let servers: http.Server[] = []; | ||
|
|
||
| afterEach(() => { | ||
| servers.forEach((server) => { | ||
| server.close(); | ||
| }); | ||
| servers = []; | ||
| }); | ||
|
|
||
| it('should return undefined when no ports are in use', async () => { | ||
| const port = await getPort(); | ||
| expect(port === undefined || typeof port === 'number').toBe(true); | ||
| if (port !== undefined) { | ||
| expect(port).toBeGreaterThan(0); | ||
| } | ||
|
|
||
| expect(port).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should handle servers listening on specific ports', async () => { | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
|
|
||
| // Listen on a specific port instead of 0 | ||
| const specificPort = 3000; | ||
| server.listen(specificPort); | ||
|
|
||
| const port = await getPort(); | ||
|
|
||
| expect(port).toEqual(specificPort); | ||
| }); | ||
|
|
||
| it('should return a port number when a server is listening', async () => { | ||
| it('should return the port number that the server is listening', async () => { | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
|
|
||
| server.listen(0); | ||
|
|
||
| try { | ||
| const port = await getPort(); | ||
| const address = server.address(); | ||
|
|
||
| // Port detection may not work immediately in all environments (CI, Docker, etc.) | ||
| // so we just verify the function returns a valid result | ||
| if (port !== undefined) { | ||
| expect(typeof port).toBe('number'); | ||
| expect(port).toBeGreaterThan(0); | ||
|
|
||
| // If we have the address, optionally verify it matches | ||
| if (address && typeof address === 'object') { | ||
| // In most cases it should match, but not required for test to pass | ||
| expect([port, undefined]).toContain(port); | ||
| } | ||
adriandlam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } finally { | ||
| await new Promise<void>((resolve, reject) => { | ||
| server.close((err) => (err ? reject(err) : resolve())); | ||
| }); | ||
| } | ||
| const port = await getPort(); | ||
| const addr = server.address() as AddressInfo; | ||
|
|
||
| expect(typeof port).toBe('number'); | ||
| expect(port).toEqual(addr.port); | ||
| }); | ||
|
|
||
| it('should return the smallest port when multiple servers are listening', async () => { | ||
| it('should return the first port of the server', async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test assumes View Details📝 Patch Detailsdiff --git a/packages/utils/src/get-port.test.ts b/packages/utils/src/get-port.test.ts
index 0dbad85..3b21784 100644
--- a/packages/utils/src/get-port.test.ts
+++ b/packages/utils/src/get-port.test.ts
@@ -45,7 +45,7 @@ describe('getPort', () => {
expect(port).toEqual(addr.port);
});
- it('should return the first port of the server', async () => {
+ it('should return the smallest port when multiple servers are listening', async () => {
const server1 = http.createServer();
const server2 = http.createServer();
servers.push(server1);
@@ -56,8 +56,10 @@ describe('getPort', () => {
const port = await getPort();
const addr1 = server1.address() as AddressInfo;
+ const addr2 = server2.address() as AddressInfo;
- expect(port).toEqual(addr1.port);
+ const smallestPort = Math.min(addr1.port, addr2.port);
+ expect(port).toEqual(smallestPort);
});
it('should return consistent results when called multiple times', async () => {
diff --git a/packages/utils/src/get-port.ts b/packages/utils/src/get-port.ts
index 96eb402..d1eb9a3 100644
--- a/packages/utils/src/get-port.ts
+++ b/packages/utils/src/get-port.ts
@@ -7,7 +7,7 @@ import { execa } from 'execa';
export async function getPort(): Promise<number | undefined> {
const { pid, platform } = process;
- let port: number | undefined;
+ let ports: number[] = [];
try {
switch (platform) {
@@ -21,14 +21,22 @@ export async function getPort(): Promise<number | undefined> {
'-p',
pid.toString(),
]);
- const awkResult = await execa(
- 'awk',
- ['/LISTEN/ {split($9,a,":"); print a[length(a)]; exit}'],
- {
- input: lsofResult.stdout,
+ const lines = lsofResult.stdout.split('\n');
+ for (const line of lines) {
+ if (line.includes('LISTEN')) {
+ const parts = line.split(/\s+/);
+ if (parts.length > 8) {
+ const addr = parts[8];
+ const portMatch = addr.match(/:(\d+)$/);
+ if (portMatch) {
+ const p = parseInt(portMatch[1], 10);
+ if (!Number.isNaN(p) && !ports.includes(p)) {
+ ports.push(p);
+ }
+ }
+ }
}
- );
- port = parseInt(awkResult.stdout.trim(), 10);
+ }
break;
}
@@ -47,8 +55,10 @@ export async function getPort(): Promise<number | undefined> {
// Extract port from the local address column
const match = line.trim().match(/^\s*TCP\s+[\d.:]+:(\d+)\s+/);
if (match) {
- port = parseInt(match[1], 10);
- break;
+ const p = parseInt(match[1], 10);
+ if (!Number.isNaN(p) && !ports.includes(p)) {
+ ports.push(p);
+ }
}
}
}
@@ -63,5 +73,10 @@ export async function getPort(): Promise<number | undefined> {
return undefined;
}
- return Number.isNaN(port) ? undefined : port;
+ if (ports.length === 0) {
+ return undefined;
+ }
+
+ // Return the smallest port for consistent behavior
+ return Math.min(...ports);
}
AnalysisTest assumes getPort() returns server1's port but implementation returns first port from lsof/netstatWhat fails: The test "should return the first port of the server" in How to reproduce: Create two HTTP servers with random ports and call getPort(): server1.listen(0);
server2.listen(0);
const port = await getPort();Since OS assigns random ports, server1 doesn't always have a lower port than server2. The original implementation used lsof/netstat which returns the "first" listening socket, but lsof output order is not guaranteed to match server creation order. The test incorrectly assumed getPort() would return server1's port specifically. Result: The test assertion Expected: Per the original implementation (commit adf0cfe), getPort() should return the smallest port for predictable behavior. Updated implementation to collect all listening ports and return Math.min(...ports). Test updated to verify the smallest port is returned, not server1's specific port. Fix implemented:
|
||
| const server1 = http.createServer(); | ||
| const server2 = http.createServer(); | ||
| servers.push(server1); | ||
| servers.push(server2); | ||
|
|
||
| server1.listen(0); | ||
| server2.listen(0); | ||
|
|
||
| const port = await getPort(); | ||
| const addr1 = server1.address() as AddressInfo; | ||
|
|
||
| expect(port).toEqual(addr1.port); | ||
| }); | ||
|
|
||
| it('should return consistent results when called multiple times', async () => { | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
| server.listen(0); | ||
|
|
||
| const port1 = await getPort(); | ||
| const port2 = await getPort(); | ||
| const port3 = await getPort(); | ||
|
|
||
| expect(port1).toEqual(port2); | ||
| expect(port2).toEqual(port3); | ||
| }); | ||
|
|
||
| it('should handle IPv6 addresses', async () => { | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
|
|
||
| try { | ||
| server.listen(0, '::1'); // IPv6 localhost | ||
| const port = await getPort(); | ||
| const addr1 = server1.address(); | ||
| const addr2 = server2.address(); | ||
|
|
||
| // Port detection may not work in all environments | ||
| if ( | ||
| port !== undefined && | ||
| addr1 && | ||
| typeof addr1 === 'object' && | ||
| addr2 && | ||
| typeof addr2 === 'object' | ||
| ) { | ||
| // Should return the smallest port | ||
| expect(port).toBeLessThanOrEqual(Math.max(addr1.port, addr2.port)); | ||
| expect(port).toBeGreaterThan(0); | ||
| } else { | ||
| // If port detection doesn't work in this environment, just pass | ||
| expect(port === undefined || typeof port === 'number').toBe(true); | ||
| } | ||
| } finally { | ||
| await Promise.all([ | ||
| new Promise<void>((resolve, reject) => { | ||
| server1.close((err) => (err ? reject(err) : resolve())); | ||
| }), | ||
| new Promise<void>((resolve, reject) => { | ||
| server2.close((err) => (err ? reject(err) : resolve())); | ||
| }), | ||
| ]); | ||
| const addr = server.address() as AddressInfo; | ||
|
|
||
| expect(port).toEqual(addr.port); | ||
| } catch { | ||
| // Skip test if IPv6 is not available | ||
| console.log('IPv6 not available, skipping test'); | ||
| } | ||
| }); | ||
|
|
||
| it('should handle multiple calls in sequence', async () => { | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
|
|
||
| server.listen(0); | ||
|
|
||
| const port1 = await getPort(); | ||
| const port2 = await getPort(); | ||
| const addr = server.address() as AddressInfo; | ||
|
|
||
| // Should return the same port each time | ||
| expect(port1).toEqual(addr.port); | ||
| expect(port2).toEqual(addr.port); | ||
| }); | ||
|
|
||
| it('should handle closed servers', async () => { | ||
| const server = http.createServer(); | ||
|
|
||
| server.listen(0); | ||
| const addr = server.address() as AddressInfo; | ||
| const serverPort = addr.port; | ||
|
|
||
| // Close the server before calling getPort | ||
| server.close(); | ||
|
|
||
| const port = await getPort(); | ||
|
|
||
| // Port should not be the closed server's port | ||
| expect(port).not.toEqual(serverPort); | ||
| }); | ||
|
|
||
| it('should handle server restart on same port', async () => { | ||
| const server1 = http.createServer(); | ||
| servers.push(server1); | ||
| server1.listen(3000); | ||
|
|
||
| const port1 = await getPort(); | ||
| expect(port1).toEqual(3000); | ||
|
|
||
| server1.close(); | ||
| servers = servers.filter((s) => s !== server1); | ||
|
|
||
| // Small delay to ensure port is released | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| const server2 = http.createServer(); | ||
| servers.push(server2); | ||
| server2.listen(3000); | ||
|
|
||
| const port2 = await getPort(); | ||
| expect(port2).toEqual(3000); | ||
| }); | ||
|
|
||
| it('should handle concurrent getPort calls', async () => { | ||
| // Workflow makes lots of concurrent getPort calls | ||
| const server = http.createServer(); | ||
| servers.push(server); | ||
| server.listen(0); | ||
adriandlam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const addr = server.address() as AddressInfo; | ||
|
|
||
| // Call getPort concurrently 10 times | ||
| const results = await Promise.all( | ||
| Array(10) | ||
| .fill(0) | ||
| .map(() => getPort()) | ||
| ); | ||
|
|
||
| // All should return the same port without errors | ||
| results.forEach((port) => { | ||
| expect(port).toEqual(addr.port); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,71 @@ | ||
| import { pidToPorts } from 'pid-port'; | ||
| import { exec, execFileSync } from 'node:child_process'; | ||
| import { promisify } from 'node:util'; | ||
|
|
||
| const execAsync = promisify(exec); | ||
|
|
||
| /** | ||
| * Gets the port number that the process is listening on. | ||
| * @returns The port number that the process is listening on, or undefined if the process is not listening on any port. | ||
| * NOTE: Can't move this to @workflow/utils because it's being imported into @workflow/errors for RetryableError (inside workflow runtime) | ||
| */ | ||
| export async function getPort(): Promise<number | undefined> { | ||
| const { pid, platform } = process; | ||
|
|
||
| let port: number | undefined; | ||
|
|
||
| try { | ||
| const pid = process.pid; | ||
| const ports = await pidToPorts(pid); | ||
| if (!ports || ports.size === 0) { | ||
| return undefined; | ||
| // Use our fallback | ||
adriandlam marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| switch (platform) { | ||
| case 'linux': | ||
| case 'darwin': { | ||
| const lsofResult = execFileSync( | ||
| 'lsof', | ||
| ['-a', '-i', '-P', '-n', '-p', pid.toString()], | ||
| { | ||
| encoding: 'utf-8', | ||
| } | ||
| ); | ||
| const awkResult = execFileSync( | ||
| 'awk', | ||
| ['/LISTEN/ {split($9,a,":"); print a[length(a)]; exit}'], | ||
| { | ||
| encoding: 'utf-8', | ||
| input: lsofResult, | ||
| } | ||
| ); | ||
| const result = { stdout: awkResult }; | ||
| port = parseInt(result.stdout.trim(), 10); | ||
| break; | ||
| } | ||
| case 'win32': { | ||
| const lsofResult = execFileSync( | ||
| 'netstat', | ||
| ['-a', '-n', '-o', pid.toString()], | ||
| { | ||
| encoding: 'utf-8', | ||
| } | ||
| ); | ||
| const awkResult = execFileSync( | ||
| 'awk', | ||
| [ | ||
| '/LISTENING/ && /${pid}/ {split($2,a,\":\"); print a[length(a)]; exit}', | ||
adriandlam marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ], | ||
| { | ||
| encoding: 'utf-8', | ||
| input: lsofResult, | ||
| } | ||
| ); | ||
| const result = { stdout: awkResult }; | ||
| port = parseInt(result.stdout.trim(), 10); | ||
| break; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // In dev, it's helpful to know why detection failed | ||
| if (process.env.NODE_ENV === 'development') { | ||
| console.debug('[getPort] Detection failed:', error); | ||
| } | ||
|
|
||
| const smallest = Math.min(...ports); | ||
| return smallest; | ||
| } catch { | ||
| // If port detection fails (e.g., `ss` command not available in production), | ||
| // return undefined and fall back to default port | ||
| return undefined; | ||
| } | ||
|
|
||
| return port || undefined; | ||
adriandlam marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
race condition between nitro apps rebuilding after dev test cleanup and e2e tests starting