Skip to content

test(api): ephemeral port for the fake orchestrator gRPC server (fix fixed-5008 flake) - #3401

Merged
tvi merged 1 commit into
mainfrom
fix/api-orchestrator-ephemeral-port
Jul 26, 2026
Merged

tvi merged 1 commit into
mainfrom
fix/api-orchestrator-ephemeral-port

Conversation

@tomassrnka

Copy link
Copy Markdown
Member

TestGetOrConnectNode_CacheMiss_DiscoversAndConnects bound a fixed port (consts.OrchestratorAPIPort = 5008) for its fake gRPC server and flaked with address already in use — on shared CI runners (parallel shards) and on dev boxes running a real orchestrator on 5008.

The helper now binds an ephemeral port and returns it; the mocked Nomad service registration carries that port, so discovery still dials the right listener.

Pre-existing flake, independent of the multi-distro work — split out of #3381 to keep that PR focused (per operator). Verified: go test ./internal/orchestrator/ -run TestGetOrConnectNode|TestConnectToNode green locally; gofmt clean.

🤖 Generated with Claude Code

TestGetOrConnectNode_CacheMiss_DiscoversAndConnects bound a fixed port
(consts.OrchestratorAPIPort, 5008) and flaked with 'address already in use' on
shared CI runners and dev boxes running a real orchestrator. The helper now binds
an ephemeral port and returns it; the mocked service registration carries that
port so nomad discovery still dials the right listener.

Pre-existing flake, independent of the multi-distro work — split out of #3381.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only change with no production code paths affected.

Overview
Fixes flaky address already in use failures in the orchestrator client discovery test by having the fake gRPC helper always listen on an ephemeral port and return that port, instead of binding to the production orchestrator API port. The end-to-end cache-miss test wires the mocked Nomad service registration to the same port so on-demand discovery still connects to the fake server when tests run in parallel on shared runners or when something else already holds the fixed port.

Reviewed by Cursor Bugbot for commit aa1d137. Bugbot is set up for automated code reviews on this repo. Configure here.

tomassrnka added a commit that referenced this pull request Jul 26, 2026
Keeps #3381 focused on multi-distro. The fixed-5008 flake fix lands via #3401 on
main; this branch rebases onto main to pick it up (CI's api shard goes green then).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — straightforward test-only fix for a real flake (fixed port 5008 collision on shared runners/dev boxes).

Extended reasoning...

Overview

Single-file, test-only change in packages/api/internal/orchestrator/client_test.go. It changes startFakeOrchestratorGRPC to always bind an ephemeral port (127.0.0.1:0) and return the chosen port, removing the old addr parameter that let callers pin it to the production consts.OrchestratorAPIPort (5008). The one caller that used the fixed-port variant, TestGetOrConnectNode_CacheMiss_DiscoversAndConnects, is updated to use the returned ephemeral port for both the gRPC listener and the mocked Nomad service registration's Port field, so discovery still dials the correct address.

Security risks

None — this only affects test code, no production code paths or data are touched.

Level of scrutiny

Low. This is a mechanical test-infra fix addressing a well-understood flake (binding a fixed, well-known port causes collisions in parallel CI shards or when a real orchestrator is already running on that port locally). The fix is the standard idiom (ephemeral port + propagate the chosen port to the mock).

Other factors

I checked that no other test in the file relied on the old two-argument signature or the fixed-port behavior — only the one updated call site used a non-empty addr. The change is consistent with the PR description and author's local verification (targeted test run + gofmt). No outstanding review comments to address; the only prior timeline entry is an automated low-risk summary from Cursor Bugbot.

@tvi
tvi enabled auto-merge (squash) July 26, 2026 20:29
@tvi
tvi merged commit 4a33482 into main Jul 26, 2026
42 checks passed
@tvi
tvi deleted the fix/api-orchestrator-ephemeral-port branch July 26, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants