Remove local health port binding, check upstream directly, and fix Docker PR CI (#5)

* Initial plan

* fix: tolerate default health port conflict

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

* refactor: make health checks internal

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

* chore: address review follow-ups

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

* chore: slow docker healthcheck cadence

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

* fix: avoid ghcr push on pr builds

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

* fix: simplify docker workflow cache logic

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

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tma <4719+tma@users.noreply.github.com>

Author: Copilot

.github/workflows/docker.yml

@@ -17,6 +17,9 @@ jobs:
     permissions:
       contents: read
       packages: write
+    env:
+      CACHE_FROM: ${{ github.event_name == 'pull_request' && 'type=gha' || format('type=registry,ref=ghcr.io/{0}:buildcache', github.repository) }}
+      CACHE_TO: ${{ github.event_name == 'pull_request' && 'type=gha,mode=max' || format('type=registry,ref=ghcr.io/{0}:buildcache,mode=max', github.repository) }}
 
     steps:
       - uses: actions/checkout@v6
@@ -55,5 +58,5 @@ jobs:
           push: ${{ github.event_name != 'pull_request' }}
           tags: ${{ steps.meta.outputs.tags }}
           labels: ${{ steps.meta.outputs.labels }}
-          cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:buildcache
-          cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:buildcache,mode=max
+          cache-from: ${{ env.CACHE_FROM }}
+          cache-to: ${{ env.CACHE_TO }}

.gitignore

@@ -1,5 +1,5 @@
 # Build artifacts
-mbproxy
+/mbproxy
 *.exe
 
 # Go specific

Dockerfile

@@ -31,9 +31,7 @@ FROM scratch
 
 COPY --from=builder /app/mbproxy /mbproxy
 
-EXPOSE 8080
-
-HEALTHCHECK --interval=5s --timeout=3s --start-period=10s --retries=3 \
+HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \
   CMD ["/mbproxy", "-health"]
 
 ENTRYPOINT ["/mbproxy"]

README.md

@@ -39,6 +39,8 @@ All configuration is via environment variables:
 | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` |
 | `LOG_LEVEL` | Log level: `INFO`, `DEBUG` | `INFO` |
 
+`/mbproxy -health` performs an internal upstream connectivity check and does not open a separate local TCP health port.
+
 ### Read-Only Modes
 
 - `false`: Full read/write passthrough to upstream device

SPEC.md

@@ -108,6 +108,8 @@ Three modes:
 | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` | `10s`, `60s` |
 | `LOG_LEVEL` | Log level | `INFO` | `INFO`, `DEBUG` |
 
+The container health check runs `mbproxy -health`, which performs an internal upstream connectivity check without binding a separate local TCP port.
+
 ## Implementation Details
 
 ### Dependencies

cmd/mbproxy/main.go

@@ -8,11 +8,10 @@ import (
 	"os"
 	"os/signal"
 	"syscall"
-	"time"
 
 	"github.com/tma/mbproxy/internal/config"
-	"github.com/tma/mbproxy/internal/health"
 	"github.com/tma/mbproxy/internal/logging"
+	"github.com/tma/mbproxy/internal/modbus"
 	"github.com/tma/mbproxy/internal/proxy"
 )
 
@@ -21,8 +20,7 @@ func main() {
 	flag.Parse()
 
 	if *healthCheck {
-		addr := config.GetEnv("HEALTH_LISTEN", ":8080")
-		if err := health.CheckHealth(addr); err != nil {
+		if err := runHealthCheck(); err != nil {
 			fmt.Fprintln(os.Stderr, err)
 			os.Exit(1)
 		}
@@ -50,19 +48,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Start health server
-	hs := health.NewServer(cfg.HealthListen, p, logger)
-	hsLn, err := hs.Listen()
-	if err != nil {
-		logger.Error("failed to start health server", "error", err)
-		os.Exit(1)
-	}
-	go func() {
-		if err := hs.Serve(hsLn); err != nil {
-			logger.Error("health server error", "error", err)
-		}
-	}()
-
 	// Start proxy in background
 	errCh := make(chan error, 1)
 	go func() {
@@ -83,16 +68,31 @@ func main() {
 	// Graceful shutdown
 	cancel()
 
-	shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer shutdownCancel()
-	if err := hs.Shutdown(shutdownCtx); err != nil {
-		logger.Error("health server shutdown error", "error", err)
-	}
-
 	if err := p.Shutdown(cfg.ShutdownTimeout); err != nil {
 		logger.Error("shutdown error", "error", err)
 		os.Exit(1)
 	}
 
 	logger.Info("shutdown complete")
 }
+
+func runHealthCheck() error {
+	cfg, err := config.Load()
+	if err != nil {
+		return err
+	}
+
+	return checkUpstreamHealth(cfg, logging.New(cfg.LogLevel))
+}
+
+func checkUpstreamHealth(cfg *config.Config, logger *slog.Logger) (err error) {
+	client := modbus.NewClient(cfg.Upstream, cfg.Timeout, cfg.RequestDelay, cfg.ConnectDelay, logger)
+	defer func() {
+		closeErr := client.Close()
+		if err == nil && closeErr != nil {
+			err = closeErr
+		}
+	}()
+
+	return client.Connect()
+}

cmd/mbproxy/main_test.go

@@ -0,0 +1,59 @@
+package main
+
+import (
+	"io"
+	"log/slog"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/tma/mbproxy/internal/config"
+)
+
+func newTestLogger() *slog.Logger {
+	return slog.New(slog.NewTextHandler(io.Discard, nil))
+}
+
+func TestCheckUpstreamHealth_Success(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+	defer ln.Close()
+
+	acceptDone := make(chan struct{})
+	go func() {
+		defer close(acceptDone)
+		conn, err := ln.Accept()
+		if err == nil {
+			conn.Close()
+		}
+	}()
+
+	cfg := &config.Config{
+		Upstream: ln.Addr().String(),
+		Timeout:  time.Second,
+	}
+	if err := checkUpstreamHealth(cfg, newTestLogger()); err != nil {
+		t.Fatalf("expected health check to succeed, got %v", err)
+	}
+
+	<-acceptDone
+}
+
+func TestCheckUpstreamHealth_Failure(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to reserve port: %v", err)
+	}
+	addr := ln.Addr().String()
+	ln.Close()
+
+	cfg := &config.Config{
+		Upstream: addr,
+		Timeout:  100 * time.Millisecond,
+	}
+	if err := checkUpstreamHealth(cfg, newTestLogger()); err == nil {
+		t.Fatal("expected health check to fail")
+	}
+}

internal/config/config.go

@@ -30,7 +30,6 @@ type Config struct {
 	RequestDelay    time.Duration
 	ConnectDelay    time.Duration
 	ShutdownTimeout time.Duration
-	HealthListen    string
 	LogLevel        string
 }
 
@@ -47,7 +46,6 @@ func Load() (*Config, error) {
 		RequestDelay:    0,
 		ConnectDelay:    0,
 		ShutdownTimeout: 30 * time.Second,
-		HealthListen:    GetEnv("HEALTH_LISTEN", ":8080"),
 		LogLevel:        GetEnv("LOG_LEVEL", "INFO"),
 	}
 

internal/config/config_test.go

@@ -19,7 +19,6 @@ func TestLoad_Defaults(t *testing.T) {
 	os.Unsetenv("MODBUS_READONLY")
 	os.Unsetenv("MODBUS_TIMEOUT")
 	os.Unsetenv("MODBUS_SHUTDOWN_TIMEOUT")
-	os.Unsetenv("HEALTH_LISTEN")
 	os.Unsetenv("LOG_LEVEL")
 
 	cfg, err := Load()
@@ -57,9 +56,6 @@ func TestLoad_Defaults(t *testing.T) {
 	if cfg.ShutdownTimeout != 30*time.Second {
 		t.Errorf("expected 30s shutdown timeout, got %v", cfg.ShutdownTimeout)
 	}
-	if cfg.HealthListen != ":8080" {
-		t.Errorf("expected :8080, got %s", cfg.HealthListen)
-	}
 	if cfg.LogLevel != "INFO" {
 		t.Errorf("expected INFO log level, got %s", cfg.LogLevel)
 	}
@@ -85,7 +81,6 @@ func TestLoad_CustomValues(t *testing.T) {
 	os.Setenv("MODBUS_REQUEST_DELAY", "100ms")
 	os.Setenv("MODBUS_CONNECT_DELAY", "200ms")
 	os.Setenv("MODBUS_SHUTDOWN_TIMEOUT", "60s")
-	os.Setenv("HEALTH_LISTEN", ":9090")
 	os.Setenv("LOG_LEVEL", "DEBUG")
 
 	defer func() {
@@ -99,7 +94,6 @@ func TestLoad_CustomValues(t *testing.T) {
 		os.Unsetenv("MODBUS_REQUEST_DELAY")
 		os.Unsetenv("MODBUS_CONNECT_DELAY")
 		os.Unsetenv("MODBUS_SHUTDOWN_TIMEOUT")
-		os.Unsetenv("HEALTH_LISTEN")
 		os.Unsetenv("LOG_LEVEL")
 	}()
 
@@ -135,9 +129,6 @@ func TestLoad_CustomValues(t *testing.T) {
 	if cfg.ShutdownTimeout != 60*time.Second {
 		t.Errorf("expected 60s shutdown timeout, got %v", cfg.ShutdownTimeout)
 	}
-	if cfg.HealthListen != ":9090" {
-		t.Errorf("expected :9090, got %s", cfg.HealthListen)
-	}
 	if cfg.LogLevel != "DEBUG" {
 		t.Errorf("expected DEBUG log level, got %s", cfg.LogLevel)
 	}