-
Notifications
You must be signed in to change notification settings - Fork 105
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
Draft
adriandlam
wants to merge
33
commits into
main
Choose a base branch
from
fix/nitro-app-workaround
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
aa41312
add debug logs
adriandlam efc7f00
add polling for port test
adriandlam dedfe1a
nuxt may be hmring on ignored files/folders
adriandlam b519d38
Revert "nuxt may be hmring on ignored files/folders"
adriandlam f74a8cf
Update packages/utils/src/get-port.ts
adriandlam e628752
Update packages/world-local/src/config.ts
adriandlam 8adf7c4
Update packages/utils/src/get-port.ts
adriandlam 7393a48
revert getPort
adriandlam f0426d2
test new pid to port cmd
adriandlam 981a796
extend sleep time on tests
adriandlam a6c8b60
fix
adriandlam ee26a58
fix race condition in tests
adriandlam 83868ed
add logs
adriandlam 1826088
add fallback from pid-port
adriandlam 0f8f6ad
remove pid-port fallback
adriandlam 48756f0
revert config ts
adriandlam 0bf618f
test: add getPort test
adriandlam 59beb55
update tests for concurrent calls
adriandlam faa91cb
temp
adriandlam 06bb751
trigger rebuild
adriandlam 5d7b1c8
feat: add windows get port support
adriandlam 506b994
fix port sorting
adriandlam 3d540e9
fix parsing logic and filtering
adriandlam 275d0d4
fformat
adriandlam 5ff5c74
update ports logi
adriandlam 7a036f2
revert
adriandlam 417274f
windows port hack
adriandlam 7ce66e5
refactor: optional chaining on windows
adriandlam 1088e9d
test: change ordering of config tests
adriandlam 23b6b76
remove wrong test
adriandlam f265a06
simplify windows getPort impl
adriandlam cbd26a6
use execFileSync instead of execFile
adriandlam 311da18
use execa
adriandlam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 () => { | ||
| 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); | ||
|
|
||
| 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); | ||
| }); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,44 @@ | ||
| import { pidToPorts } from 'pid-port'; | ||
| import { exec } 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 | ||
| switch (platform) { | ||
| case 'linux': | ||
| case 'darwin': { | ||
| // Grab the first port entry reported | ||
| const result = await execAsync( | ||
| `lsof -a -i -P -n -p ${pid} | awk '/LISTEN/ {split($9,a,":"); print a[length(a)]; exit}'` | ||
| ); | ||
| port = parseInt(result.stdout.trim(), 10); | ||
| break; | ||
| } | ||
| case 'win32': { | ||
| const result = await execAsync( | ||
| `netstat -ano | awk "/LISTENING/ && /${pid}/ {split($2,a,\":\"); print a[length(a)]; exit}"` | ||
| ); | ||
| 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; | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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