Skip to content

Restore HTTP-based health server, make it opt-in#6

Merged
tma merged 2 commits intomainfrom
fix/restore-http-healthcheck
Mar 11, 2026
Merged

Restore HTTP-based health server, make it opt-in#6
tma merged 2 commits intomainfrom
fix/restore-http-healthcheck

Conversation

@tma
Copy link
Copy Markdown
Owner

@tma tma commented Mar 11, 2026

Revert the direct-upstream health probe introduced in #5, which opened a second TCP connection to the Modbus device on every check. Many Modbus devices only accept one concurrent connection, so the probe interfered with normal proxy operation in production.

Restore the original passive HTTP health server that checks internal connection state without touching upstream. Make it opt-in via HEALTH_LISTEN so bare-binary users running multiple instances (the root cause of #4) no longer hit port conflicts. The Dockerfile sets HEALTH_LISTEN=:8080 so Docker users get health checks automatically.

Closes #4

Revert the direct-upstream health probe introduced in #5, which opened
a second TCP connection to the Modbus device on every check. Many
Modbus devices only accept one concurrent connection, so the probe
interfered with normal proxy operation in production.

Restore the original passive HTTP health server that checks internal
connection state without touching upstream. Make it opt-in via
HEALTH_LISTEN so bare-binary users running multiple instances (the
root cause of #4) no longer hit port conflicts. The Dockerfile sets
HEALTH_LISTEN=:8080 so Docker users get health checks automatically.

Closes #4
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

This PR restores the original HTTP-based (passive) health mechanism and makes the health listener opt-in via HEALTH_LISTEN, preventing multi-instance port conflicts while keeping Docker health checks enabled by default.

Changes:

  • Add HEALTH_LISTEN to configuration (default empty) and wire it into mbproxy startup to conditionally run an internal HTTP /health server.
  • Update -health mode to perform an HTTP check against the configured health listener (instead of dialing upstream).
  • Update Docker image defaults to enable the health listener on :8080 and expose port 8080.

Reviewed changes

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

Show a summary per file
File Description
internal/config/config.go Adds HealthListen to config loaded from HEALTH_LISTEN.
internal/config/config_test.go Extends config tests to cover default empty HealthListen and custom HEALTH_LISTEN.
cmd/mbproxy/main.go Replaces upstream-dial health check with HTTP-based health check and conditionally starts/shuts down health server.
cmd/mbproxy/main_test.go Removes tests for the previous upstream-dial health check implementation.
Dockerfile Enables opt-in health server for Docker via HEALTH_LISTEN=:8080, exposes 8080, and updates the container healthcheck.

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

@tma
Copy link
Copy Markdown
Owner Author

tma commented Mar 11, 2026

@copilot open a new pull request to apply changes based on the comments in this thread

Co-authored-by: tma <4719+tma@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 11, 2026

@copilot open a new pull request to apply changes based on the comments in this thread

Both review items are addressed in commit c9c4fde:

  1. CheckHealth wildcard/IPv6 handling — now normalises "", 0.0.0.0, and :: to localhost, and builds the URL with net.JoinHostPort so IPv6 literals get the required brackets (e.g. http://[::1]:8080/health). New tests TestCheckHealth_WildcardAddresses and TestCheckHealth_IPv6Loopback cover these cases.

  2. TestLoad_HealthListenCustom isolation — converted to t.Setenv and added explicit clears for all optional env vars that Load() reads, matching the pattern used in TestLoad_Defaults.

Copilot finished work on behalf of tma March 11, 2026 00:13
@tma tma merged commit 4915499 into main Mar 11, 2026
2 checks passed
@tma tma deleted the fix/restore-http-healthcheck branch March 11, 2026 00:15
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.

error="listen tcp :8080: bind: address already in use"

3 participants