Skip to content

Make discovery server optional via Option<u16> port#17

Merged
RReverser merged 5 commits into
RReverser:mainfrom
ivonnyssen:pr/optional-discovery-server
Apr 19, 2026
Merged

Make discovery server optional via Option<u16> port#17
RReverser merged 5 commits into
RReverser:mainfrom
ivonnyssen:pr/optional-discovery-server

Conversation

@ivonnyssen
Copy link
Copy Markdown
Contributor

Summary

Changes Server's discovery port from u16 to Option<u16>, allowing the discovery server to be disabled entirely by passing None. Also makes DEFAULT_DISCOVERY_PORT public so consumers can reference it.

Motivation

When running multiple Alpaca servers in integration tests, each server tries to bind the discovery UDP socket, causing port conflicts. Making the discovery server optional allows test servers to skip discovery binding entirely, avoiding these conflicts.

Changes

  • src/server/mod.rs:
    • Server::discovery_port changed from u16 to Option<u16>
    • BoundServer::discovery changed from BoundDiscoveryServer to Option<BoundDiscoveryServer>
    • BoundServer::discovery_listen_addr() returns Option<SocketAddr>
    • BoundServer::start() restructured to handle both cases
  • src/discovery.rs: DEFAULT_DISCOVERY_PORT changed from pub(crate) to pub with documentation

Test plan

  • cargo clippy passes (full CI feature matrix verified)
  • Server::new(...) with Some(port) behaves identically to before
  • Server::new(...) with None skips discovery server entirely
  • DEFAULT_DISCOVERY_PORT accessible from downstream crates

@RReverser
Copy link
Copy Markdown
Owner

@ivonnyssen Can you please mark PRs that are ready for review as such? Looks like there's a bunch now in draft mode, and Github requires them to be merged "ready for review" before I can approve :)

@RReverser
Copy link
Copy Markdown
Owner

When running multiple Alpaca servers in integration tests, each server tries to bind the discovery UDP socket, causing port conflicts.

Hm I thought I ran tests in parallel with no issues so far... What error are you getting? Would set_reuse_port help instead?

@ivonnyssen
Copy link
Copy Markdown
Contributor Author

When running multiple Alpaca servers in integration tests, each server tries to bind the discovery UDP socket, causing port conflicts.

Hm I thought I ran tests in parallel with no issues so far... What error are you getting? Would set_reuse_port help instead?

The issues are on MacOS. There the set_reuse_port does not help, as the os is holding on to the port for a bit, making tests either very slow - wait for port to be re-usable - or forcing them to be completely sequential.

@ivonnyssen ivonnyssen marked this pull request as ready for review April 12, 2026 20:24
Copilot AI review requested due to automatic review settings April 12, 2026 20:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the server configuration to allow disabling the UDP discovery responder by making the discovery port optional, primarily to avoid port-binding conflicts when running multiple servers (e.g., in integration tests).

Changes:

  • Changed Server::discovery_port from u16 to Option<u16> with None disabling discovery.
  • Updated BoundServer to hold Option<BoundDiscoveryServer> and return Option<SocketAddr> from discovery_listen_addr().
  • Made DEFAULT_DISCOVERY_PORT public and documented it for downstream use.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/server/mod.rs Makes discovery binding optional and threads Option through the bound server APIs and startup logic.
src/discovery.rs Exposes DEFAULT_DISCOVERY_PORT publicly with rustdoc.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/server/mod.rs Outdated
Comment thread src/server/mod.rs
RReverser and others added 4 commits April 19, 2026 10:19
Change Server.discovery_port from u16 to Option<u16>. When set to None,
the discovery server is not started, which is useful for tests and
environments where ASCOM discovery is not needed.

This also fixes the dead field bug where Server.discovery_port was never
actually used — Server::bind() always hardcoded DEFAULT_DISCOVERY_PORT
(32227) via DiscoveryServer::for_alpaca_server_at(). Now the configured
port is passed through to the discovery server.

Breaking change: discovery_port is now Option<u16> (default Some(32227)).
discovery_listen_addr() now returns Option<SocketAddr>.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Expose the ASCOM Alpaca default discovery port (32227) as a public
constant so downstream crates can reference it instead of duplicating
the magic number.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Replace redundant closure with method reference for discovery_listen_addr
- Restructure if-let/else to match for start() to satisfy redundant_else lint
@RReverser RReverser force-pushed the pr/optional-discovery-server branch from 6219465 to 5e155bd Compare April 19, 2026 09:22
@RReverser RReverser force-pushed the pr/optional-discovery-server branch from 5e155bd to 300301f Compare April 19, 2026 09:23
@RReverser RReverser merged commit c9ef4d5 into RReverser:main Apr 19, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants