Skip to content

Retry dual-era backend startup on a lost port-bind race - #6080

Merged
JAORMX merged 1 commit into
stacklok:mainfrom
jhrozek:fix-dual-era-port-bind-race
Jul 28, 2026
Merged

Retry dual-era backend startup on a lost port-bind race#6080
JAORMX merged 1 commit into
stacklok:mainfrom
jhrozek:fix-dual-era-port-bind-race

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Why: test/e2e/vmcp_dual_era_test.go's BeforeEach intermittently fails
    waiting for a yardstick backend to become running, with the container
    erroring out as driver failed programming external connectivity ... bind: address already in use. allocateVMCPPort (vmcp_cli_helpers_test.go)
    probes for a free port and closes its own listener immediately, but the
    actual bind happens later, asynchronously, inside the container Docker
    starts for the detached thv run process — under back-to-back workload
    churn, another container can grab the same ephemeral port first. This is a
    classic check-then-use race across process boundaries; the only way to
    make progress in the face of it is to retry, not to check earlier/later.
  • What: startYardstickLegacyOnPort/startYardstickModernOnPort now go
    through a shared startEraBackendOnPort helper that retries once with a
    freshly allocated port if WaitForMCPServer fails with
    "address already in use".

Type of change

  • Bug fix

Test plan

  • E2E tests (task test-e2e)
  • Manual testing (describe below)

Ran ginkgo --label-filter='vmcp && dual-era' --repeat=20 -v locally against
this fix. All 20 repeats (140 specs) passed. The port race fired once during
the run (visible in the log as lost a port-bind race on <port>, retrying with a fresh port) and was absorbed by the retry instead of failing the
spec — confirming both that the race is real and that the fix works.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Does this introduce a user-facing change?

No. Test-only change.

Special notes for reviewers

This fix is scoped to the two call sites that actually race Docker's async
container bind. allocateVMCPPort is also used elsewhere for thv vmcp serve's own port, which is bound synchronously in-process by the same test
binary and isn't subject to this race, so I left allocateVMCPPort itself
unchanged rather than touching its other callers.

Generated with Claude Code

@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.24%. Comparing base (e8f84ef) to head (1a0802c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6080      +/-   ##
==========================================
- Coverage   72.26%   72.24%   -0.03%     
==========================================
  Files         728      728              
  Lines       75523    75523              
==========================================
- Hits        54580    54561      -19     
- Misses      17040    17057      +17     
- Partials     3903     3905       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

allocateVMCPPort probes for a free port and closes its own listener
immediately, but the actual bind happens later, asynchronously, inside
the container Docker starts for the detached `thv run` process. Under
back-to-back workload churn another container can grab the same
ephemeral port first, failing with "address already in use" and
leaving the spec's BeforeEach to time out after 2 minutes.

Retry once with a freshly allocated port when this specific failure is
observed. Confirmed live under a 20-repeat stress run: the race still
fires (~1 in 12-13 attempts), but the retry now absorbs it instead of
failing the spec.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jhrozek
jhrozek force-pushed the fix-dual-era-port-bind-race branch from 5838b25 to 1a0802c Compare July 28, 2026 09:12
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 28, 2026
@JAORMX
JAORMX merged commit e2c4441 into stacklok:main Jul 28, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants