Skip to content

fix(common-server): add listener bind to prevent blocking on multiport setup - #648

Open
stefan-ctrl wants to merge 6 commits into
mainfrom
fix-common-server-multiport-blocking
Open

fix(common-server): add listener bind to prevent blocking on multiport setup#648
stefan-ctrl wants to merge 6 commits into
mainfrom
fix-common-server-multiport-blocking

Conversation

@stefan-ctrl

@stefan-ctrl stefan-ctrl commented Sep 2, 2026

Copy link
Copy Markdown
Member

Goal

Fix the Timeout bug as seen in main: https://github.com/telekom/controlplane/actions/runs/33595609763/job/100138458027#step:6:58

Description

Issue

pkg/server tests could hang for 10 minutes:

  • MultiServer.Run bound each listener lazily inside its own goroutine (app.Listen(addr)), while a separate goroutine shuts down all apps exactly once when ctx is cancelled.
  • If one listener's port was already in use, its bind failed instantly and cancelled ctx.
  • If that happened before a sibling goroutine even got scheduled, Shutdown() was a no-op on it (fiber only supports shutdown once serving has started).
  • The sibling then later bound and blocked in Accept() forever.

Root Cause

According to AI:
This race has existed since MultiServer was introduced in #507 and just never lost the coin flip in CI Verified across ~30 recent runs — all green, ~5s each.

Personal Note:
Seems kinda unlikely that nothing has changed and a "coin flip" was always in our favor, since it now fails constantly.
However, I would not consider it worth it to explore potential root causes even further, and instead fix this issue and move on.

@stefan-ctrl stefan-ctrl added go Pull requests that update go code stability Improves stability in some way labels Sep 2, 2026
@stefan-ctrl stefan-ctrl self-assigned this Sep 2, 2026
@stefan-ctrl
stefan-ctrl marked this pull request as ready for review September 2, 2026 06:57
Copilot AI lite review requested due to automatic review settings September 2, 2026 06:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new preflight check is skipped for TLS listeners even though production uses TLS MultiServer and serve.ServeTLS still binds inside the goroutine, so the same hang scenario can still occur.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR aims to prevent common-server/pkg/server multiport server tests (and potentially servers) from hanging when one listener fails to bind by adding a synchronous preflight bind-check before starting any serve goroutines.

Changes:

  • Add a preflight net.Listen/Close loop for plain-HTTP listeners to fail fast on bind errors before goroutines are spawned.
  • Introduce net import and return a contextual error (listener "<addr>": ...) on preflight bind failure.
File summaries
File Description
common-server/pkg/server/multiport.go Adds a preflight bind-check intended to prevent a shutdown race/hang when one of multiple listeners cannot bind.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread common-server/pkg/server/multiport.go Outdated
@stefan-ctrl

Copy link
Copy Markdown
Member Author

@ron96g ron96g left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really minor bug that probably cannot really happen in prod, unless we misconfigure ports. Just one comment

Comment thread common-server/pkg/server/multiport.go Outdated
@stefan-ctrl
stefan-ctrl requested a review from ron96g September 2, 2026 08:40
@stefan-ctrl stefan-ctrl changed the title fix(common-server): add listener bind-check to prevent blocking on multiport setup fix(common-server): add listener bind to prevent blocking on multiport setup Sep 2, 2026

@ron96g ron96g left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

some minor things. Mainly I would use the net.Listener over the addr in most cases

Comment thread common-server/pkg/server/multiport.go
Comment thread common-server/pkg/server/serve/serve.go Outdated
Comment thread common-server/pkg/server/multiport.go Outdated
@stefan-ctrl
stefan-ctrl requested a review from ron96g September 2, 2026 13:28

@ron96g ron96g left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code stability Improves stability in some way

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants