fix(config/loader): close old services before binding new ports on reload (issue #754)

Services are the only component group that binds a port during parsing
(ParseService calls listener.Init). The generic registerGroup "parse-all-
then-swap" sequence bound every new listener while the old services were
still listening, causing EADDRINUSE on SIGHUP reload — a regression from
82e7e50.

Close and unregister all old services first (unregisterAll relies on
registry.Unregister closing io.Closer values, which frees each service's
port), then parse, bind, and register the new ones. This restores the
pre-82e7e50 close-old-before-bind ordering for the services group.

config/loader/loader.go

@@ -23,7 +23,6 @@ import (
 	"github.com/go-gost/core/resolver"
 	"github.com/go-gost/core/router"
 	"github.com/go-gost/core/sd"
-	"github.com/go-gost/core/service"
 	"github.com/go-gost/x/config"
 	"github.com/go-gost/x/config/parsing"
 	admission_parser "github.com/go-gost/x/config/parsing/admission"
@@ -94,14 +93,12 @@ type named[T any] struct {
 	v    T
 }
 
-// registerGroup replaces all entries in r with the given entries.
+// registerGroup replaces all entries in r with the given entries. Old entries
-// Old entries are unregistered first; if any new entry fails to register,
+// are unregistered first (see unregisterAll); if any new entry fails to
-// the group is left partially updated (matching the historical reload
+// register, the group is left partially updated (matching the historical
-// behavior for intra-group failures).
+// reload behavior for intra-group failures).
 func registerGroup[T any](entries []named[T], r reg.Registry[T]) error {
-	for name := range r.GetAll() {
+	unregisterAll(r)
-		r.Unregister(name)
-	}
 	for _, e := range entries {
 		if err := r.Register(e.name, e.v); err != nil {
 			return err
@@ -110,6 +107,15 @@ func registerGroup[T any](entries []named[T], r reg.Registry[T]) error {
 	return nil
 }
 
+// unregisterAll removes every entry from r. registry.Unregister closes a value
+// before deleting it when it implements io.Closer, so for services this frees
+// the bound port (a service's Close closes its listener).
+func unregisterAll[T any](r reg.Registry[T]) {
+	for name := range r.GetAll() {
+		r.Unregister(name)
+	}
+}
+
 // register parses config sections and registers them into the global
 // registries. Groups are processed in dependency order: leaf components
 // first, then hops, chains, and finally services. This ensures that
@@ -300,21 +306,34 @@ func register(cfg *config.Config) error {
 
 	// --- services (references chains, resolvers, hosts, recorders,
 	//     limiters, observers, hops from registries) ---
-
+	//
+	// Services are the only component group whose construction binds a port:
+	// service_parser.ParseService calls listener.Init, which binds. The generic
+	// registerGroup "parse-all-then-swap" sequence used by the other groups
+	// would therefore bind every new listener while the old services are still
+	// listening, causing EADDRINUSE on SIGHUP reload (issue #754, regressed by
+	// 82e7e50). Instead, unregister all old services first — unregisterAll
+	// closes each one (a service implements io.Closer), freeing its port —
+	// then parse, bind, and register the new ones.
+	//
+	// Trade-off: a parse error in the loop below leaves the registry partially
+	// updated (the old services are already closed and only the services parsed
+	// before the failure are registered). The construct/bind split would be
+	// needed for atomic reload, but this restores the pre-82e7e50 behavior.
 	{
-		var entries []named[service.Service]
+		unregisterAll(registry.ServiceRegistry())
+
 		for _, c := range cfg.Services {
 			svc, err := service_parser.ParseService(c)
 			if err != nil {
 				return err
 			}
 			if svc != nil {
-				entries = append(entries, named[service.Service]{c.Name, svc})
+				if err := registry.ServiceRegistry().Register(c.Name, svc); err != nil {
+					return err
+				}
 			}
 		}
-		if err := registerGroup(entries, registry.ServiceRegistry()); err != nil {
-			return err
-		}
 	}
 
 	return nil

config/loader/loader_test.go

@@ -3,8 +3,10 @@ package loader
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"net"
+	"strings"
 	"testing"
 
 	"github.com/go-gost/core/handler"
@@ -805,3 +807,129 @@ func TestRegister_ServiceUnknownListener(t *testing.T) {
 		t.Fatal("expected error for unknown listener type, got nil")
 	}
 }
+
+// bindingStubListener is a stubListener whose Init actually binds a TCP port
+// via net.Listen, reproducing the EADDRINUSE condition the close-old-before-
+// bind ordering in register() must prevent. Used only by reload-collision tests.
+type bindingStubListener struct {
+	addr string
+	ln   net.Listener
+}
+
+func (l *bindingStubListener) Init(md metadata.Metadata) error {
+	ln, err := net.Listen("tcp", l.addr)
+	if err != nil {
+		return err
+	}
+	l.ln = ln
+	return nil
+}
+
+func (l *bindingStubListener) Accept() (net.Conn, error) {
+	if l.ln == nil {
+		return nil, net.ErrClosed
+	}
+	return l.ln.Accept()
+}
+
+func (l *bindingStubListener) Addr() net.Addr {
+	if l.ln != nil {
+		return l.ln.Addr()
+	}
+	return &net.TCPAddr{IP: net.IPv4zero, Port: 0}
+}
+
+func (l *bindingStubListener) Close() error {
+	if l.ln == nil {
+		return nil
+	}
+	return l.ln.Close()
+}
+
+// freeTCPPort returns the number of a TCP port that is free at call time by
+// opening and immediately closing a listener on 127.0.0.1:0. There is a small
+// race window before the caller rebinds, which is acceptable for tests.
+func freeTCPPort(t *testing.T) int {
+	t.Helper()
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen for free port: %v", err)
+	}
+	addr := l.Addr().(*net.TCPAddr)
+	l.Close()
+	return addr.Port
+}
+
+// TestRegister_ServiceReloadNoCollision verifies that calling register() twice
+// with a service on a fixed port does NOT return EADDRINUSE on the second call
+// (issue #754). Before the close-old-before-bind ordering, the second
+// register() bound the new listener while the old one was still listening.
+func TestRegister_ServiceReloadNoCollision(t *testing.T) {
+	port := freeTCPPort(t)
+	addr := fmt.Sprintf("127.0.0.1:%d", port)
+
+	// Register a binding listener factory under a test-specific name so we
+	// don't interfere with the "tcp" save/restore in TestRegister_Services.
+	const factoryName = "tcp-binding-test"
+	origListener := registry.ListenerRegistry().Get(factoryName)
+	registry.ListenerRegistry().Register(factoryName, func(opts ...listener.Option) listener.Listener {
+		options := listener.Options{}
+		for _, opt := range opts {
+			opt(&options)
+		}
+		return &bindingStubListener{addr: options.Addr}
+	})
+	// Reuse the inert "auto" handler stub.
+	origHandler := registry.HandlerRegistry().Get("auto")
+	registry.HandlerRegistry().Register("auto", func(opts ...handler.Option) handler.Handler {
+		return &stubHandler{}
+	})
+	t.Cleanup(func() {
+		registry.ListenerRegistry().Unregister(factoryName)
+		registry.HandlerRegistry().Unregister("auto")
+		if origListener != nil {
+			registry.ListenerRegistry().Register(factoryName, origListener)
+		}
+		if origHandler != nil {
+			registry.HandlerRegistry().Register("auto", origHandler)
+		}
+	})
+
+	r := registry.ServiceRegistry()
+	const svcName = "reload-svc"
+	t.Cleanup(func() { r.Unregister(svcName) })
+
+	buildCfg := func() *config.Config {
+		return &config.Config{
+			Services: []*config.ServiceConfig{
+				{
+					Name:     svcName,
+					Addr:     addr,
+					Listener: &config.ListenerConfig{Type: factoryName},
+					Handler:  &config.HandlerConfig{Type: "auto"},
+				},
+			},
+		}
+	}
+
+	// First load: registers the service and binds the port.
+	if err := register(buildCfg()); err != nil {
+		t.Fatalf("first register: unexpected error: %v", err)
+	}
+	if !r.IsRegistered(svcName) {
+		t.Fatal("expected service registered after first load")
+	}
+
+	// Second load simulates a SIGHUP reload. With the old ordering this bound
+	// the new listener while the old one was still open and returned
+	// "address already in use"; the fix closes old services first.
+	if err := register(buildCfg()); err != nil {
+		if strings.Contains(err.Error(), "address already in use") {
+			t.Fatalf("second register (reload): port collision not avoided: %v", err)
+		}
+		t.Fatalf("second register (reload): unexpected error: %v", err)
+	}
+	if !r.IsRegistered(svcName) {
+		t.Fatal("expected service registered after reload")
+	}
+}