Fix: PUP_CLUSTER_PORT not set when commonPort is omitted#79
Conversation
Fix error handling
Fix #21, auto start services after install on windows
…mmonPort Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the PUP_CLUSTER_PORT environment variable was not being set for cluster instances when commonPort was omitted. Previously, the variable assignment was incorrectly gated on the presence of commonPort, causing all instances to use the same default port and resulting in EADDRINUSE errors when using external load balancers like nginx.
Changes:
- Decoupled
PUP_CLUSTER_PORTenvironment variable assignment fromcommonPortin the cluster initialization logic - Added test coverage for the fix verifying that
PUP_CLUSTER_PORTis correctly set per instance when onlystartPortis defined
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/core/cluster.ts | Updated logic to set PUP_CLUSTER_PORT whenever startPort is defined, regardless of whether the built-in load balancer (commonPort) is active |
| test/core/pup.test.ts | Added test verifying PUP_CLUSTER_PORT is set correctly for each cluster instance when startPort is defined without commonPort |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -133,14 +134,25 @@ test("Cluster sets PUP_CLUSTER_PORT for each instance without common port", asyn | |||
| "cmd": TEST_PROCESS_COMMAND, | |||
| "cluster": { | |||
| "instances": 2, | |||
| "startPort": 8000, | |||
| "startPort": START_PORT, | |||
| }, | |||
| }, | |||
| ], | |||
| } | |||
| const pup = new Pup(config) | |||
| await pup.init() | |||
|
|
|||
| // Find the cluster process | |||
| const testCluster = pup.processes.findLast((p) => p.getConfig().id === TEST_PROCESS_ID) | |||
| assertNotEquals(testCluster, undefined) | |||
| assertEquals(testCluster instanceof Cluster, true) | |||
|
|
|||
| const cluster = testCluster as Cluster | |||
|
|
|||
| // Each sub-process should have PUP_CLUSTER_PORT set to startPort + index | |||
| assertEquals(cluster.processes.length, 2) | |||
| assertEquals(cluster.processes[0].getConfig().env?.PUP_CLUSTER_PORT, String(START_PORT)) | |||
| assertEquals(cluster.processes[1].getConfig().env?.PUP_CLUSTER_PORT, String(START_PORT + 1)) | |||
There was a problem hiding this comment.
This test duplicates coverage already provided in test/core/cluster.test.ts at line 13, which tests the exact same scenario: "Cluster with startPort but without commonPort should set PUP_CLUSTER_PORT". The cluster.test.ts version is more thorough, using a loop to verify all instances and testing with 3 instances instead of 2. Since pup.test.ts is for blackbox integration tests of Pup lifecycle behavior (start/stop/block/unblock), cluster-specific configuration tests belong in cluster.test.ts.
There was a problem hiding this comment.
Removed the duplicated test from pup.test.ts and cleaned up the unused imports (Cluster, assert). The coverage in cluster.test.ts is more thorough (3 instances, loops over all, checks both with and without commonPort). Commit: dfab675
|
@copilot apply changes based on the comments in this thread |
|
@copilot fix tests, formatting etc (ci) |
…ter.test.ts) Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
cluster.ts: SetPUP_CLUSTER_PORTenv var whenstartPortis defined, regardless ofcommonPort(fixes issue Deno.env not updated when commonPort omitted in nginx setup #71)pup.test.ts— cluster-specific config tests are incluster.test.ts💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.