fix(test): fix parallel test server isolation — remove shared ctx.port fallback#743
fix(test): fix parallel test server isolation — remove shared ctx.port fallback#743
Conversation
asclearuc
requested review from
Rod-Christensen,
jmaionchi,
kwit75 and
stepmikhaylov
as code owners
📝 WalkthroughWalkthroughRemoved the Changes
Sequence DiagramsequenceDiagram
actor Workflow as GitHub Workflow
participant Launcher as Test Server Launcher
participant Bracket as Bracket Registry
participant Runner as Test Runner (pytest/jest)
participant Server as Test Server
Workflow->>Launcher: start test server (--port=0)
Launcher->>Bracket: register server (port, serverUri)
alt bracket contains port/serverUri
Runner->>Bracket: read bracket for module
Runner->>Runner: compute ROCKETRIDE_URI = bracket.serverUri or http://localhost:port
Runner->>Server: connect and run tests
else missing port
Runner->>Runner: throw Error("... bracket missing — server did not start")
end
Server-->>Runner: test responses
Runner-->>Workflow: report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-python/scripts/tasks.js (1)
188-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
ROCKETRIDE_URIbefore reusing it here.
makeStartTestServerActionstill accepts a port-lessROCKETRIDE_URIand falls back to5565, but this code forwards the rawserverUriunchanged. That sends pytest to port 80 when the debug URI has no explicit port.🔧 Proposed fix
- const serverUri = bracket.serverUri || `http://localhost:${bracket.port}`; + const serverUri = + bracket.serverUri && new URL(bracket.serverUri).port + ? bracket.serverUri + : `http://localhost:${bracket.port}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-python/scripts/tasks.js` around lines 188 - 195, The code forwards a raw serverUri into testEnv.ROCKETRIDE_URI which can be a hostname-only URI (causing pytest to use port 80); update the logic around bracket/serverUri (in the makeStartTestServerAction flow) to normalize the URI: parse serverUri (or process.env.ROCKETRIDE_URI if set), ensure it includes a scheme and an explicit port (default to 5565 when none is present), and then set testEnv.ROCKETRIDE_URI to the normalized URI so consumers always get a host:port URL; change references to bracket.serverUri / serverUri accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-mcp/scripts/tasks.js`:
- Around line 111-133: The code forwards bracket.serverUri directly to
ROCKETRIDE_URI, which can be missing a port and cause pytest to target port 80;
before setting serverUri (the variable used in the env passed to execCommand),
normalize it by ensuring it has a scheme (prefix with "http://" if missing),
parse it with URL, and if url.port is empty set url.port = "5565" (the same
fallback used by makeStartTestServerAction), then use url.toString() for
ROCKETRIDE_URI; update the code around the bracket/serverUri handling and the
env passed to execCommand so the normalized serverUri is used.
---
Outside diff comments:
In `@packages/client-python/scripts/tasks.js`:
- Around line 188-195: The code forwards a raw serverUri into
testEnv.ROCKETRIDE_URI which can be a hostname-only URI (causing pytest to use
port 80); update the logic around bracket/serverUri (in the
makeStartTestServerAction flow) to normalize the URI: parse serverUri (or
process.env.ROCKETRIDE_URI if set), ensure it includes a scheme and an explicit
port (default to 5565 when none is present), and then set testEnv.ROCKETRIDE_URI
to the normalized URI so consumers always get a host:port URL; change references
to bracket.serverUri / serverUri accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 932cde7d-ee3a-4b9c-b046-ce90aa40675b
📒 Files selected for processing (5)
.github/workflows/_build.yamlnodes/scripts/tasks.jspackages/client-mcp/scripts/tasks.jspackages/client-python/scripts/tasks.jspackages/client-typescript/scripts/tasks.js
|
Heads up — this branch has a merge conflict against develop. Once you rebase / merge develop in (it now has both #734's |
asclearuc
force-pushed
the
fix/parallel-test-server-isolation
branch
from
1aa0c6d to
4e00ce9
Compare
asclearuc
force-pushed
the
fix/parallel-test-server-isolation
branch
from
1a9d3a5 to
788d812
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/client-mcp/scripts/tasks.js (1)
153-190: 🧹 Nitpick | 🔵 Trivial
client-mcpandclient-pythonsharebasePort: 20000— potential remaining parallelism contention.This PR removes the
ctx.portcross-module fallback and claims parallel execution is now safe, but bothclient-mcp(makeStartTestServerActionline 177) andpackages/client-python/scripts/tasks.js(makeStartTestServerActionline 149) start their engines with--base_port=20000.client-typescriptuses30000andnodesuses40000— explicitly separated ranges. If the engine allocates internal sub-services sequentially from--base_port, the two modules sharing20000can still collide during parallel runs.Consider assigning a distinct base port (e.g.
25000) to one of the two modules before removing-s, or verifying in CI that no sub-service conflicts occur.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-mcp/scripts/tasks.js` around lines 153 - 190, makeStartTestServerAction starts the engine with basePort: 20000 which conflicts with packages/client-python that uses the same base; change the base port here (in makeStartTestServerAction in packages/client-mcp/scripts/tasks.js) to a different non-overlapping value (e.g. 25000) or otherwise choose a distinct range, and ensure the corresponding counterpart (makeStartTestServerAction in packages/client-python/scripts/tasks.js) is adjusted or CI assertions are added to verify no sub-service port collisions; update any documentation/tests that assume basePort 20000..github/workflows/_build.yaml (1)
127-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale
:5565comment was not updated — this was explicitly called out in the PR description.The comment still reads
"Integration tests boot a local server on :5565"but each module now dynamically binds to an OS-assigned port (--port=0). This should reflect the per-module dynamic allocation introduced by this PR.📝 Proposed fix
- # Integration tests boot a local server on :5565 and connect a test - # client. Both sides need the same shared secret to authenticate; + # Integration tests boot per-module servers on dynamically assigned + # ports and connect a test client. Both sides need the same shared secret to authenticate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/_build.yaml around lines 127 - 128, Update the stale comment that mentions a fixed port ":5565" to reflect the new per-module dynamic port allocation (e.g., note that modules bind with "--port=0" / OS-assigned ports); specifically edit the comment text in .github/workflows/_build.yaml replacing the phrase "Integration tests boot a local server on :5565" with wording that indicates each module dynamically binds to an OS-assigned port (or uses "--port=0") and ensure it still explains both server and test client must share the same secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-python/scripts/tasks.js`:
- Around line 189-191: The current serverUri selection uses bracket.serverUri
truthiness which lets port-less URIs (like "http://localhost") silently target
port 80; update the logic in makeStartTestServerAction to detect an explicit
port on bracket.serverUri (use new URL(bracket.serverUri).port) and only accept
bracket.serverUri when URL.port is non-empty; otherwise fall back to
`http://localhost:${bracket.port}` so pytest uses the actual bracket.port.
Ensure you handle invalid URL parsing (try/catch) and still fall back to
bracket.port if parsing fails.
---
Outside diff comments:
In @.github/workflows/_build.yaml:
- Around line 127-128: Update the stale comment that mentions a fixed port
":5565" to reflect the new per-module dynamic port allocation (e.g., note that
modules bind with "--port=0" / OS-assigned ports); specifically edit the comment
text in .github/workflows/_build.yaml replacing the phrase "Integration tests
boot a local server on :5565" with wording that indicates each module
dynamically binds to an OS-assigned port (or uses "--port=0") and ensure it
still explains both server and test client must share the same secret.
In `@packages/client-mcp/scripts/tasks.js`:
- Around line 153-190: makeStartTestServerAction starts the engine with
basePort: 20000 which conflicts with packages/client-python that uses the same
base; change the base port here (in makeStartTestServerAction in
packages/client-mcp/scripts/tasks.js) to a different non-overlapping value (e.g.
25000) or otherwise choose a distinct range, and ensure the corresponding
counterpart (makeStartTestServerAction in
packages/client-python/scripts/tasks.js) is adjusted or CI assertions are added
to verify no sub-service port collisions; update any documentation/tests that
assume basePort 20000.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f118979b-1f89-4e62-9687-47027632c772
📒 Files selected for processing (6)
.github/workflows/_build.yaml.prettierignorenodes/scripts/tasks.jspackages/client-mcp/scripts/tasks.jspackages/client-python/scripts/tasks.jspackages/client-typescript/scripts/tasks.js
| if (!bracket?.port) throw new Error('py-test-server bracket missing — server did not start'); | ||
| // Use existing server URI when set (e.g. ROCKETRIDE_URI=http://localhost:5678 for debugging) | ||
| const serverUri = bracket?.serverUri || `http://localhost:${port}`; | ||
| const serverUri = bracket.serverUri || `http://localhost:${bracket.port}`; |
There was a problem hiding this comment.
Port-less bracket.serverUri silently routes pytest to port 80.
makeStartTestServerAction returns { ..., serverUri: envUri } verbatim from process.env.ROCKETRIDE_URI (lines 134–139). If a developer passes ROCKETRIDE_URI=http://localhost (no explicit port), bracket.serverUri is a truthy non-empty string, so the || \http://localhost:${bracket.port}\`` fallback is never reached. Pytest ends up targeting port 80 instead of the bracket.port value (e.g. 5565) that the setup action correctly derived.
The same pattern was flagged for client-mcp in a prior review; client-python has the identical bug.
🛡️ Proposed fix
- const serverUri = bracket.serverUri || `http://localhost:${bracket.port}`;
+ const serverUri =
+ bracket.serverUri && new URL(bracket.serverUri).port
+ ? bracket.serverUri
+ : `http://localhost:${bracket.port}`;URL.port returns an empty string when the port is the default for the protocol, so new URL('http://localhost').port is '' (falsy), correctly triggering the fallback for any port-less URI.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!bracket?.port) throw new Error('py-test-server bracket missing — server did not start'); | |
| // Use existing server URI when set (e.g. ROCKETRIDE_URI=http://localhost:5678 for debugging) | |
| const serverUri = bracket?.serverUri || `http://localhost:${port}`; | |
| const serverUri = bracket.serverUri || `http://localhost:${bracket.port}`; | |
| if (!bracket?.port) throw new Error('py-test-server bracket missing — server did not start'); | |
| // Use existing server URI when set (e.g. ROCKETRIDE_URI=http://localhost:5678 for debugging) | |
| const serverUri = | |
| bracket.serverUri && new URL(bracket.serverUri).port | |
| ? bracket.serverUri | |
| : `http://localhost:${bracket.port}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-python/scripts/tasks.js` around lines 189 - 191, The current
serverUri selection uses bracket.serverUri truthiness which lets port-less URIs
(like "http://localhost") silently target port 80; update the logic in
makeStartTestServerAction to detect an explicit port on bracket.serverUri (use
new URL(bracket.serverUri).port) and only accept bracket.serverUri when URL.port
is non-empty; otherwise fall back to `http://localhost:${bracket.port}` so
pytest uses the actual bracket.port. Ensure you handle invalid URL parsing
(try/catch) and still fall back to bracket.port if parsing fails.
2 participants
Summary
|| ctx.portshared-context fallback with a hard assertion in all four test modules (client-typescript,client-python,client-mcp,nodes); each module already starts its own server on a dynamically assigned port (--port=0), but the fallback silently routed subprocesses to whichever module's port was written last to the shared Listr2 context-ssequential workaround from CI (added in ci(test): run builder test sequentially to isolate cross-suite flakes #734) — parallel execution is now safe by construction:5565comment in_build.yamlto reflect dynamic per-module port allocationType
fix
Testing
./builder testpassesChecklist
Linked Issue
Fixes #741
Summary by CodeRabbit