From e744e62d03718e5b07100cc49124327a6e7da3d4 Mon Sep 17 00:00:00 2001 From: Emil Date: Mon, 3 Aug 2026 18:17:56 +0300 Subject: [PATCH] Fix network URLs in serve: loopback agents, own-origin public URLs, longer transfer timeouts --- coordinator/cmd/coordinator/serve_cmd.go | 59 +++++++++++++++---- coordinator/cmd/coordinator/serve_cmd_test.go | 22 +++++++ coordinator/internal/agent/client.go | 8 ++- coordinator/internal/agent/client_test.go | 14 +++++ 4 files changed, 92 insertions(+), 11 deletions(-) create mode 100644 coordinator/cmd/coordinator/serve_cmd_test.go diff --git a/coordinator/cmd/coordinator/serve_cmd.go b/coordinator/cmd/coordinator/serve_cmd.go index 1478930..99eecc2 100644 --- a/coordinator/cmd/coordinator/serve_cmd.go +++ b/coordinator/cmd/coordinator/serve_cmd.go @@ -10,6 +10,7 @@ import ( "flag" "fmt" "log/slog" + "net" "os" "os/exec" "path/filepath" @@ -96,18 +97,16 @@ func runServe(args []string) error { defer func() { _ = closeUsers() }() // 5. Local worker agents before the server, so they can claim immediately. - coordinatorURL := "http://" + *addr - agents, err := spawnAgents(ctx, log, *dataDir, *workers, coordinatorURL, workerToken, venvPython) + // They always dial the loopback address: an --addr of 0.0.0.0 is not a + // connectable target from the same host. + agentURL, resolvedPublic := serveURLs(*addr, *publicURL) + agents, err := spawnAgents(ctx, log, *dataDir, *workers, agentURL, workerToken, venvPython) if err != nil { return err } defer stopAgents(agents) // 6. The coordinator server itself. - coordinatorPublicURL := *publicURL - if coordinatorPublicURL == "" { - coordinatorPublicURL = "http://" + *addr - } cfg := infra.Config{ Addr: *addr, DatabaseEngine: "sqlite", @@ -115,8 +114,10 @@ func runServe(args []string) error { Token: workerToken, JWTSecret: jwtSecret, UserserviceURL: "http://" + usersAddr, - PublicCoordinatorURL: coordinatorPublicURL, - PublicUserserviceURL: "http://" + usersAddr, + PublicCoordinatorURL: resolvedPublic, + // The exchange is fronted by the coordinator's own proxy, so the UI + // falls back to the coordinator origin for USERSERVICE_URL. + PublicUserserviceURL: "", LogLevel: "info", StorageDir: filepath.Join(*dataDir, "artifacts"), DocsDir: *docsDir, @@ -132,12 +133,13 @@ func runServe(args []string) error { WorkerOfflineAfter: 1 * time.Minute, AutoMigrate: true, } + browserURL, _ := serveURLs(*addr, *publicURL) if *open { - openBrowser("http://" + *addr + "/ui/admin") + openBrowser(browserURL + "/ui/admin") } // Print the login once the server is about to start. - fmt.Printf("\nSciMesh is starting at http://%s/ui\n", *addr) + fmt.Printf("\nSciMesh is starting at %s/ui\n", browserURL) fmt.Printf(" admin login: %s / %s\n", *email, *password) if runtimeStatus(venvPython) { fmt.Printf(" scientific runtime: ready (%s)\n", venvPython) @@ -337,3 +339,40 @@ func openBrowser(target string) { // #nosec G204 -- target is the local UI URL the operator asked to open. _ = exec.CommandContext(context.Background(), command, target).Start() } + +// serveURLs derives the two addresses of a serve instance from the listen +// address and the optional --public-url flag: +// +// - the agent URL is always the loopback form of the port, because spawned +// local workers share the host and 0.0.0.0 is not connectable from it; +// - the public URL is what browsers and remote workers are told. An explicit +// --public-url wins; a listen host that is a real address is used as-is; +// a wildcard host (0.0.0.0, ::, or empty) yields an empty public URL, so +// the UI falls back to the browser's own origin (the coordinator's LAN +// address as the browser sees it). +func serveURLs(addr, publicURL string) (agentURL, resolvedPublic string) { + host, port, err := net.SplitHostPort(addr) + if err != nil { + // No port in the listen address: assume the default and treat the + // whole string as a host (e.g. a bare wildcard). + host, port = addr, "8080" + } + agentURL = "http://127.0.0.1:" + port + if publicURL != "" { + return agentURL, publicURL + } + host = strings.Trim(host, "[]") + switch host { + case "", "0.0.0.0", "::": + return agentURL, "" + default: + return agentURL, "http://" + addr + } +} + +// browserHost normalises the URL used to open the UI in the browser: the +// loopback address is what the browser on the same host must dial, and a +// wildcard host cannot be dialled at all. +func browserHost(agentURL string) string { + return agentURL +} diff --git a/coordinator/cmd/coordinator/serve_cmd_test.go b/coordinator/cmd/coordinator/serve_cmd_test.go new file mode 100644 index 0000000..0dfc97d --- /dev/null +++ b/coordinator/cmd/coordinator/serve_cmd_test.go @@ -0,0 +1,22 @@ +package main + +import "testing" + +func TestServeURLs(t *testing.T) { + cases := []struct { + addr, public, agent, resolved string + }{ + {"127.0.0.1:8080", "", "http://127.0.0.1:8080", "http://127.0.0.1:8080"}, + {"0.0.0.0:8080", "", "http://127.0.0.1:8080", ""}, + {":8080", "", "http://127.0.0.1:8080", ""}, + {"::", "", "http://127.0.0.1:8080", ""}, + {"192.168.1.10:8080", "", "http://127.0.0.1:8080", "http://192.168.1.10:8080"}, + {"0.0.0.0:8080", "http://cluster.example:8080", "http://127.0.0.1:8080", "http://cluster.example:8080"}, + } + for _, c := range cases { + agent, resolved := serveURLs(c.addr, c.public) + if agent != c.agent || resolved != c.resolved { + t.Errorf("serveURLs(%q, %q) = (%q, %q), want (%q, %q)", c.addr, c.public, agent, resolved, c.agent, c.resolved) + } + } +} diff --git a/coordinator/internal/agent/client.go b/coordinator/internal/agent/client.go index 1087f84..4469a48 100644 --- a/coordinator/internal/agent/client.go +++ b/coordinator/internal/agent/client.go @@ -48,6 +48,12 @@ type Client struct { } func NewClient(baseURL string, tokens TokenProvider, timeout time.Duration) *Client { + // Payload transfers get a more generous budget than control calls: a large + // shard over a slow link easily outlives the API timeout. + transferTimeout := timeout * 4 + if transferTimeout < 2*time.Minute { + transferTimeout = 2 * time.Minute + } return &Client{ baseURL: strings.TrimRight(baseURL, "/"), tokens: tokens, @@ -57,7 +63,7 @@ func NewClient(baseURL string, tokens TokenProvider, timeout time.Duration) *Cli CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, }, dlClient: &http.Client{ - Timeout: timeout, + Timeout: transferTimeout, CheckRedirect: func(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return fmt.Errorf("too many redirects") diff --git a/coordinator/internal/agent/client_test.go b/coordinator/internal/agent/client_test.go index f78ec31..8e21ae5 100644 --- a/coordinator/internal/agent/client_test.go +++ b/coordinator/internal/agent/client_test.go @@ -223,3 +223,17 @@ func sha256Of(t *testing.T, value string) string { digest := sha256.Sum256([]byte(value)) return fmt.Sprintf("%x", digest) } + +func TestNewClientTransferTimeoutExceedsAPITimeout(t *testing.T) { + c := NewClient("http://coord:8080", &StaticToken{token: "t"}, 30*time.Second) + if c.apiClient.Timeout != 30*time.Second { + t.Errorf("api timeout = %v, want 30s", c.apiClient.Timeout) + } + if c.dlClient.Timeout < 2*time.Minute { + t.Errorf("transfer timeout = %v, want at least 2m", c.dlClient.Timeout) + } + short := NewClient("http://coord:8080", &StaticToken{token: "t"}, 3*time.Minute) + if short.dlClient.Timeout != 12*time.Minute { + t.Errorf("transfer timeout = %v, want 4x the api timeout", short.dlClient.Timeout) + } +}