From a6c7bfad63898e97a6e0c15b028e15420f1d5009 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:53:34 +0000 Subject: [PATCH 1/4] feat(server): reload configuration without a restart via --reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gomodel --reload` now asks the running gateway to re-read its configuration, the same operation as `nginx -s reload`. It resolves the new `server.pid_file` / `PID_FILE` setting, signals that process (SIGHUP, so `kill -HUP` works too), and exits. On the signal the gateway re-reads the `.env` file and then reloads the whole configuration by rebuilding itself, so every setting reloads rather than a curated subset. Two properties make it safe to run in production: - the replacement is built before the running one is stopped, so a configuration that fails to load or initialize leaves the gateway serving on the one that already works; - the listening socket is owned by the process and handed to each generation in turn, so requests arriving mid-reload wait in the accept queue instead of being refused. Environment file handling keeps startup's precedence: variables exported into the process still win over the file, edited values are applied, and variables removed from the file are unset. `PORT` and `PID_FILE` changes still need a restart and are warned about. The pid file defaults to `data/gomodel.pid` next to an existing `./data` directory and to the OS per-user data directory otherwise — the same resolution the SQLite database uses. Closes #573 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty --- .env.template | 6 + CLAUDE.md | 1 + config/config.example.yaml | 1 + config/config.go | 1 + config/server.go | 28 ++++ config/server_test.go | 66 ++++++++ docs/advanced/cli.mdx | 85 +++++++++- docs/advanced/config-yaml.mdx | 5 + docs/advanced/configuration.mdx | 1 + run/flags.go | 2 + run/lifecycle_test.go | 43 ++--- run/reload.go | 152 +++++++++++++++++ run/reload_test.go | 279 ++++++++++++++++++++++++++++++++ run/run.go | 209 ++++++++++++++++++++---- run/socket.go | 76 +++++++++ 15 files changed, 898 insertions(+), 57 deletions(-) create mode 100644 config/server_test.go create mode 100644 run/reload.go create mode 100644 run/reload_test.go create mode 100644 run/socket.go diff --git a/.env.template b/.env.template index 1605c1fc5..58d420cf9 100644 --- a/.env.template +++ b/.env.template @@ -5,6 +5,12 @@ # Header used to read/write request user_path values (default: X-GoModel-User-Path) # USER_PATH_HEADER=X-GoModel-User-Path +# Where the running gateway records its process id so `gomodel --reload` can find it +# (default: data/gomodel.pid next to a ./data directory, otherwise the per-user data +# directory). Set it per instance when several gateways share a host; set it empty to +# write no pid file, which also disables --reload. +# PID_FILE=data/gomodel.pid + # Reject unknown keys in config.yaml and in the JSON env vars that declare the same # structures (VIRTUAL_MODELS, SET_RATE_LIMIT_*, SET_BUDGET_*). Default: true, so a # typo or a misindented section fails startup instead of silently dropping providers, diff --git a/CLAUDE.md b/CLAUDE.md index c1031f4a4..2d0bf1bff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,6 +111,7 @@ Full reference: `.env.template` and `config/config.yaml` - `GOMODEL_MASTER_KEY` (empty = unsafe mode). Managed API keys (dashboard API Keys page / `POST /admin/auth-keys`) carry a per-key `dashboard_access` flag (default false, changeable via `PUT /admin/auth-keys/{id}/dashboard-access`): only the master key and flagged keys can call the admin REST API endpoints under `/admin/*` (others get 403 `dashboard_access_denied`); the dashboard UI shell and static assets (`/admin/dashboard`, `/admin/static/*`) skip auth entirely — only the admin data they load is gated; model endpoints and `GET /v1/usage` stay open to every key, and the no-master-key lockout-recovery path (auth skipped on `/admin/*`) is unaffected. - `BODY_SIZE_LIMIT` ("10M") - `USER_PATH_HEADER` (`X-GoModel-User-Path`: Header used to read/write request `user_path` values) + - `PID_FILE` / `server.pid_file` (`data/gomodel.pid` next to a `./data` directory, otherwise the OS per-user data dir — same resolution as `SQLITE_PATH`): where the running gateway records its process id. `gomodel --reload` reads it and signals that process (SIGHUP; `kill -HUP` works too) to reload configuration without a restart, like `nginx -s reload`. The reload re-reads `.env` (exported variables still win over the file; variables removed from the file are unset) and the whole config, then rebuilds the application — so every setting reloads, not a curated subset. The replacement is built before the running one is stopped, so a broken config keeps the current one serving; the listening socket is held across generations, so no connection is refused mid-reload. `PORT` and `PID_FILE` changes still need a restart (warned about), and in-memory state — rate limit counters, session affinity pins, live log buffers — resets as it would on restart. Empty disables the pid file and `--reload`. Not available on Windows (POSIX signals). - `ENABLE_PASSTHROUGH_ROUTES` (true: Enable provider-native passthrough routes under /p/{provider}/...) - `ALLOW_PASSTHROUGH_V1_ALIAS` (true: Allow /p/{provider}/v1/... aliases while keeping /p/{provider}/... canonical) - `ENABLED_PASSTHROUGH_PROVIDERS` (openai,anthropic,openrouter,zai,vllm: Comma-separated list of enabled passthrough providers) diff --git a/config/config.example.yaml b/config/config.example.yaml index 585168c4a..98e37f85c 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -15,6 +15,7 @@ server: user_path_header: "X-GoModel-User-Path" # env: USER_PATH_HEADER; inbound header used for user_path scoping enabled_passthrough_providers: ["openai", "anthropic", "cohere", "openrouter", "kilo", "zai", "vllm", "deepseek", "bailian"] # providers enabled on /p/{provider}/... realtime_enabled: true # env: REALTIME_ENABLED; expose /v1/realtime websocket and /p/{provider}/v1/realtime upgrades (OpenAI only) + pid_file: "data/gomodel.pid" # env: PID_FILE; where the running gateway records its process id so `gomodel --reload` can find it. Set per instance when several gateways share a host; empty writes no pid file and disables --reload models: enabled_by_default: true # env: MODELS_ENABLED_BY_DEFAULT; when false, models stay unavailable until an access override allows one or more user paths diff --git a/config/config.go b/config/config.go index 9225c768f..865ebc9a1 100644 --- a/config/config.go +++ b/config/config.go @@ -61,6 +61,7 @@ func buildDefaultConfig() *Config { UserPathHeader: "X-GoModel-User-Path", SwaggerEnabled: false, PprofEnabled: false, + PIDFile: DefaultPIDFilePath(), EnablePassthroughRoutes: true, AllowPassthroughV1Alias: true, RealtimeEnabled: true, diff --git a/config/server.go b/config/server.go index 39edad4a2..200efa955 100644 --- a/config/server.go +++ b/config/server.go @@ -3,10 +3,14 @@ package config import ( "fmt" "net/textproto" + "os" "path" + "path/filepath" "regexp" "strconv" "strings" + + "github.com/enterpilot/gomodel/internal/platformdir" ) // Body size limit constants @@ -43,6 +47,30 @@ type ServerConfig struct { // at /v1/realtime and the /p/{provider}/v1/realtime passthrough upgrade. // Default: true. Only providers implementing realtime accept sessions. RealtimeEnabled bool `yaml:"realtime_enabled" env:"REALTIME_ENABLED"` + // PIDFile records the process id of the running gateway so `gomodel --reload` + // can find it. Default: DefaultPIDFilePath(). Set it per instance when + // several gateways share a host; set it empty to write no pid file, which + // also disables `--reload`. + PIDFile string `yaml:"pid_file" env:"PID_FILE"` +} + +// LegacyPIDFilePath is the pid file location used next to a project-local +// ./data directory, matching where the SQLite database lands in the same setup. +const LegacyPIDFilePath = "data/gomodel.pid" + +// DefaultPIDFilePath returns the pid file path used when none is configured: +// LegacyPIDFilePath when a ./data directory already exists (Docker images and +// existing deployments), otherwise the OS-conventional per-user data directory +// — the same resolution storage.DefaultSQLitePath uses for the database. +func DefaultPIDFilePath() string { + if info, err := os.Stat("data"); err == nil && info.IsDir() { + return LegacyPIDFilePath + } + dir, err := platformdir.DataDir() + if err != nil { + return LegacyPIDFilePath + } + return filepath.Join(dir, "gomodel.pid") } var headerNameRegex = regexp.MustCompile(`^[!#$%&'*+\-.^_` + "`" + `|~0-9A-Za-z]+$`) diff --git a/config/server_test.go b/config/server_test.go new file mode 100644 index 000000000..b7dfaae51 --- /dev/null +++ b/config/server_test.go @@ -0,0 +1,66 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/enterpilot/gomodel/internal/platformdir" +) + +// The pid file has to land next to the database, or `gomodel --reload` looks +// for it somewhere the gateway never wrote it: a Docker image with /app/data +// keeps both project-local, a binary install keeps both in the per-user data +// directory. +func TestDefaultPIDFilePath(t *testing.T) { + platformDataDir, err := platformdir.DataDir() + if err != nil { + t.Fatalf("platformdir.DataDir() error: %v", err) + } + + tests := []struct { + name string + setup func(t *testing.T, dir string) + want string + }{ + { + name: "data directory exists keeps the project-local path", + setup: func(t *testing.T, dir string) { + if err := os.Mkdir(filepath.Join(dir, "data"), 0o755); err != nil { + t.Fatal(err) + } + }, + want: LegacyPIDFilePath, + }, + { + name: "no data directory uses the platform path", + setup: func(t *testing.T, dir string) {}, + want: filepath.Join(platformDataDir, "gomodel.pid"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + tt.setup(t, dir) + t.Chdir(dir) + + if got := DefaultPIDFilePath(); got != tt.want { + t.Errorf("DefaultPIDFilePath() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestPIDFileEnvOverride(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv("PID_FILE", "/var/run/gomodel/custom.pid") + + result, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if got := result.Config.Server.PIDFile; got != "/var/run/gomodel/custom.pid" { + t.Errorf("Server.PIDFile = %q, want the PID_FILE value", got) + } +} diff --git a/docs/advanced/cli.mdx b/docs/advanced/cli.mdx index 621d23c8e..1f4b1e4f5 100644 --- a/docs/advanced/cli.mdx +++ b/docs/advanced/cli.mdx @@ -1,8 +1,8 @@ --- title: "CLI Operations" -description: "Command-line flags for inspecting the GoModel binary and probing a running gateway's health." +description: "Command-line flags for inspecting the GoModel binary, probing a running gateway's health, and reloading its configuration." icon: "terminal" -keywords: ["CLI", "command-line flags", "health check", "version probe"] +keywords: ["CLI", "command-line flags", "health check", "version probe", "config reload", "SIGHUP"] --- ## Overview @@ -18,6 +18,7 @@ the examples below use the long form. | `--health-timeout` | Maximum time to wait for the `--health` probe | `2s` | | `--ready` | Probe the local `/health/ready` (readiness) endpoint and exit | — | | `--ready-timeout` | Maximum time to wait for the `--ready` probe | `4s` | +| `--reload` | Tell the running gateway to reload its configuration and exit | — | ## Version @@ -90,6 +91,86 @@ gomodel --ready { "status": "ready", "components": { "storage": "ok", "cache": "ok" } } ``` +## Configuration reload + +`--reload` applies configuration changes to a running gateway without +restarting it — the same operation as `nginx -s reload`: + +```bash +gomodel --reload +``` + +It reads the pid file, signals that process, and exits. The gateway then: + +1. Re-reads the `.env` file. New and edited values are applied; values removed + from the file are unset. Variables exported into the process environment + keep winning over the file, exactly as they do at startup — a container's + environment is not overridden by a file inside it. +2. Re-reads `config/config.yaml` (or `config.yaml`) and every environment + variable, then rebuilds itself from the result. Providers, virtual models, + budgets, rate limits, guardrails, MCP servers, caching, logging, admin + settings — all of it reloads, because the reload re-runs the same startup + path rather than a hand-picked subset. + +The replacement is built **before** the running configuration is stopped, so a +configuration that fails to load or initialize changes nothing: the gateway logs +`reload failed; keeping the running configuration` and keeps serving on what +already works. + +The listening socket is held for the lifetime of the process and handed to each +configuration in turn, so requests arriving mid-reload wait to be accepted +rather than being refused. In-flight requests are drained first (see +`GracefulDrainTimeout`), and streamed responses that outlive the drain window +are cut, so reload during heavy streaming traffic is not free — but no +connection is dropped at the socket. + +Sending the signal directly does the same thing, which is what a process manager +or a container without a shell can use: + +```bash +kill -HUP "$(cat data/gomodel.pid)" +``` + + + Reload is a POSIX signal feature and is not available on Windows. + + +### The pid file + +The gateway writes its process id to `PID_FILE` / `server.pid_file` at startup +and removes it on shutdown. The default is `data/gomodel.pid` when a `./data` +directory exists (Docker images and existing deployments) and the +OS-conventional per-user data directory otherwise — the same resolution the +SQLite database uses. + +Give each instance its own path when several gateways share a host, since +`--reload` signals whichever process the file names. Setting it empty writes no +pid file and disables `--reload`; `kill -HUP` still works. If the path is not +writable the gateway logs a warning and serves normally, only without `--reload` +support. + +Both the gateway and `gomodel --reload` resolve the path from the same +configuration, so run the command from the same working directory (or with the +same `PID_FILE`) as the gateway: + +```bash +docker exec my-gateway /gomodel --reload +``` + +### What a reload does not change + +- **`PORT`** — the socket stays bound so no connection is refused; a port change + needs a restart. The gateway logs a warning naming both ports. +- **`PID_FILE`** — it names the process that is already running. +- **`GOMODEL_DEMO_MODE`** — the demo warnings are wired up once at startup. +- **In-memory state** — rate limit counters, virtual-model session affinity, and + live log buffers start fresh, exactly as they would after a restart. Budgets + and usage are stored in the database and are unaffected. + +For refreshing provider model catalogs and admin-managed data *without* re-reading +configuration, the dashboard's runtime refresh (`POST /admin/runtime/refresh`) is +the lighter option. + Liveness (`--health`) is the right signal for a Docker `HEALTHCHECK` (restart on crash). Readiness (`/health/ready`) is the right signal for a Kubernetes `readinessProbe` (gate traffic) — point it at the HTTP endpoint directly or run diff --git a/docs/advanced/config-yaml.mdx b/docs/advanced/config-yaml.mdx index 641511f97..80eb9b0f3 100644 --- a/docs/advanced/config-yaml.mdx +++ b/docs/advanced/config-yaml.mdx @@ -110,6 +110,11 @@ For deployments mounted below a domain root, set `server.base_path` or To change the inbound user path header, set `server.user_path_header` or `USER_PATH_HEADER`. The default remains `X-GoModel-User-Path`. +Edits to this file are applied to a running gateway with `gomodel --reload`, +which re-reads the file and the environment and rebuilds the gateway in place — +see [CLI Operations](/advanced/cli#configuration-reload). Nothing changes if the +new file fails to load. + ## Docker GoModel reads `config/config.yaml` first, then `config.yaml`. diff --git a/docs/advanced/configuration.mdx b/docs/advanced/configuration.mdx index 95518e168..1a96f8af5 100644 --- a/docs/advanced/configuration.mdx +++ b/docs/advanced/configuration.mdx @@ -47,6 +47,7 @@ The most common way to configure GoModel. Set any of the variables below to over | `GOMODEL_DEMO_MODE` | Enable public demo warnings in logs and the dashboard | `false` | | `BODY_SIZE_LIMIT` | Max request body size (e.g., `10M`, `1024K`, `500KB`) | _(no limit)_ | | `USER_PATH_HEADER` | Header used to read/write request `user_path` values | `X-GoModel-User-Path` | +| `PID_FILE` | Where the running gateway records its process id for `gomodel --reload` | `data/gomodel.pid` next to a `./data` directory, otherwise the per-user data directory | Set `GOMODEL_DEMO_MODE=true` for a public or shared demonstration instance. GoModel logs a warning at startup and every five minutes, renders a persistent diff --git a/run/flags.go b/run/flags.go index 2d2f14b7c..8aa075233 100644 --- a/run/flags.go +++ b/run/flags.go @@ -21,6 +21,7 @@ type cliOptions struct { HealthTimeout time.Duration Ready bool ReadyTimeout time.Duration + Reload bool } func parseCLI(productName string, args []string, output io.Writer) (cliOptions, error) { @@ -32,6 +33,7 @@ func parseCLI(productName string, args []string, output io.Writer) (cliOptions, flags.DurationVar(&opts.HealthTimeout, "health-timeout", defaultHealthTimeout, "Timeout for --health") flags.BoolVar(&opts.Ready, "ready", false, "Check the local GoModel readiness endpoint and exit") flags.DurationVar(&opts.ReadyTimeout, "ready-timeout", defaultReadyTimeout, "Timeout for --ready") + flags.BoolVar(&opts.Reload, "reload", false, "Tell the running GoModel to reload its configuration and exit") if err := flags.Parse(args); err != nil { return opts, err } diff --git a/run/lifecycle_test.go b/run/lifecycle_test.go index 56dc28200..26bf2c598 100644 --- a/run/lifecycle_test.go +++ b/run/lifecycle_test.go @@ -3,6 +3,7 @@ package run import ( "context" "errors" + "net" "slices" "sync" "sync/atomic" @@ -24,7 +25,7 @@ type stubLifecycleApp struct { shutdownBlock <-chan struct{} } -func (s *stubLifecycleApp) Start(_ context.Context, _ string) error { +func (s *stubLifecycleApp) StartWithListener(_ context.Context, _ net.Listener) error { s.mu.Lock() s.startCalls++ s.mu.Unlock() @@ -63,11 +64,11 @@ func (s *stubLifecycleApp) capturedShutdownContext() context.Context { // A server that never came up still holds a database handle and whatever the // loggers buffered while it was being built, so it gets torn down — once, on // one shutdownTimeout budget, from the same place every other exit uses. -func TestServeUntilShutdown_TearsDownOnceAfterAFailedStart(t *testing.T) { +func TestServeGeneration_TearsDownOnceAfterAFailedStart(t *testing.T) { startErr := errors.New("listen tcp :8080: bind: address already in use") app := &stubLifecycleApp{startErr: startErr} - err := serveUntilShutdown(context.Background(), app, ":8080") + err := serveGeneration(context.Background(), app, nil) if !errors.Is(err, startErr) { t.Fatalf("error = %v, want start error %v", err, startErr) } @@ -92,11 +93,11 @@ func TestServeUntilShutdown_TearsDownOnceAfterAFailedStart(t *testing.T) { // The start error is what the operator needs to see and what sets the exit // code, so a teardown that also fails is logged rather than wrapped around it. -func TestServeUntilShutdown_ShutdownFailureDoesNotMaskTheStartError(t *testing.T) { +func TestServeGeneration_ShutdownFailureDoesNotMaskTheStartError(t *testing.T) { startErr := errors.New("listen failed") app := &stubLifecycleApp{startErr: startErr, shutdownErr: errors.New("close failed")} - err := serveUntilShutdown(context.Background(), app, ":8080") + err := serveGeneration(context.Background(), app, nil) if !errors.Is(err, startErr) { t.Fatalf("error = %v, want start error %v", err, startErr) } @@ -106,8 +107,8 @@ func TestServeUntilShutdown_ShutdownFailureDoesNotMaskTheStartError(t *testing.T } // A teardown that wedges must not wedge the process with it: the wait is -// bounded by shutdownTimeout and serveUntilShutdown returns regardless. -func TestServeUntilShutdown_StopsWaitingWhenShutdownTimesOut(t *testing.T) { +// bounded by shutdownTimeout and serveGeneration returns regardless. +func TestServeGeneration_StopsWaitingWhenShutdownTimesOut(t *testing.T) { previousTimeout := shutdownTimeout shutdownTimeout = 10 * time.Millisecond defer func() { @@ -122,7 +123,7 @@ func TestServeUntilShutdown_StopsWaitingWhenShutdownTimesOut(t *testing.T) { done := make(chan error, 1) go func() { - done <- serveUntilShutdown(context.Background(), app, ":8080") + done <- serveGeneration(context.Background(), app, nil) }() select { @@ -131,7 +132,7 @@ func TestServeUntilShutdown_StopsWaitingWhenShutdownTimesOut(t *testing.T) { t.Fatalf("error = %v, want start error %v", err, startErr) } case <-time.After(5 * time.Second): - t.Fatal("serveUntilShutdown blocked on a shutdown that never returned") + t.Fatal("serveGeneration blocked on a shutdown that never returned") } if calls := app.shutdownCallCount(); calls != 1 { t.Fatalf("shutdownCalls = %d, want 1", calls) @@ -168,7 +169,7 @@ func newServingApp() *servingApp { } } -func (a *servingApp) Start(context.Context, string) error { +func (a *servingApp) StartWithListener(context.Context, net.Listener) error { <-a.serverStopped return nil } @@ -184,14 +185,14 @@ func (a *servingApp) Shutdown(context.Context) error { // flushing loses whatever it had not written yet. That is what happened on // every Ctrl+C: the server stopped, Start returned, the process left, and // "application shutdown complete" was never reached. -func TestServeUntilShutdown_WaitsForTeardownToFinish(t *testing.T) { +func TestServeGeneration_WaitsForTeardownToFinish(t *testing.T) { app := newServingApp() ctx, cancel := context.WithCancel(context.Background()) defer cancel() returned := make(chan error, 1) go func() { - returned <- serveUntilShutdown(ctx, app, ":0") + returned <- serveGeneration(ctx, app, nil) }() cancel() // the SIGINT equivalent @@ -199,7 +200,7 @@ func TestServeUntilShutdown_WaitsForTeardownToFinish(t *testing.T) { // Start has returned by now; Shutdown is still flushing. select { case err := <-returned: - t.Fatalf("serveUntilShutdown returned mid-teardown (error = %v)", err) + t.Fatalf("serveGeneration returned mid-teardown (error = %v)", err) case <-time.After(100 * time.Millisecond): } @@ -207,10 +208,10 @@ func TestServeUntilShutdown_WaitsForTeardownToFinish(t *testing.T) { select { case err := <-returned: if err != nil { - t.Fatalf("serveUntilShutdown() error = %v, want nil", err) + t.Fatalf("serveGeneration() error = %v, want nil", err) } case <-time.After(5 * time.Second): - t.Fatal("serveUntilShutdown did not return after teardown finished") + t.Fatal("serveGeneration did not return after teardown finished") } if !app.shutdownDone.Load() { t.Fatal("teardown did not run to completion") @@ -219,23 +220,23 @@ func TestServeUntilShutdown_WaitsForTeardownToFinish(t *testing.T) { // A server that stops without a signal still owns a database handle and // buffered records, so it gets the same teardown. -func TestServeUntilShutdown_TearsDownWhenServerStopsOnItsOwn(t *testing.T) { +func TestServeGeneration_TearsDownWhenServerStopsOnItsOwn(t *testing.T) { app := &stubLifecycleApp{} - if err := serveUntilShutdown(context.Background(), app, ":0"); err != nil { - t.Fatalf("serveUntilShutdown() error = %v, want nil", err) + if err := serveGeneration(context.Background(), app, nil); err != nil { + t.Fatalf("serveGeneration() error = %v, want nil", err) } if calls := app.shutdownCallCount(); calls != 1 { t.Fatalf("shutdownCalls = %d, want 1", calls) } } -func TestServeUntilShutdown_ReturnsStartFailure(t *testing.T) { +func TestServeGeneration_ReturnsStartFailure(t *testing.T) { startErr := errors.New("listen tcp :8080: bind: address already in use") app := &stubLifecycleApp{startErr: startErr} - if err := serveUntilShutdown(context.Background(), app, ":8080"); !errors.Is(err, startErr) { - t.Fatalf("serveUntilShutdown() error = %v, want start error %v", err, startErr) + if err := serveGeneration(context.Background(), app, nil); !errors.Is(err, startErr) { + t.Fatalf("serveGeneration() error = %v, want start error %v", err, startErr) } } diff --git a/run/reload.go b/run/reload.go new file mode 100644 index 000000000..e56d762f3 --- /dev/null +++ b/run/reload.go @@ -0,0 +1,152 @@ +package run + +import ( + "errors" + "fmt" + "io" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + + "github.com/joho/godotenv" + + "github.com/enterpilot/gomodel/config" +) + +// reloadSignal asks a running gateway to re-read its configuration, the same +// way SIGHUP does for nginx. `gomodel --reload` sends it; `kill -HUP ` +// works just as well. +const reloadSignal = syscall.SIGHUP + +// envFile is the environment file loaded at startup and re-read on reload. +const envFile = ".env" + +// dotenv applies envFile to the process environment and remembers which +// variables it set. A reload can then pick up edited values, and drop the ones +// removed from the file, without clobbering variables that came from the real +// environment: a value already exported wins over the file, exactly as it does +// at startup. +type dotenv struct { + applied map[string]string +} + +func newDotenv() *dotenv { + return &dotenv{applied: make(map[string]string)} +} + +// apply merges the current contents of envFile into the process environment. +// A missing file is normal — configuration may come entirely from the real +// environment — and clears whatever the file previously contributed. +func (d *dotenv) apply() { + values, err := godotenv.Read(envFile) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) { + slog.Warn("failed to read env file; keeping the current environment", "file", envFile, "error", err) + return + } + values = map[string]string{} + } + + for key, value := range values { + if _, owned := d.applied[key]; !owned { + if _, exported := os.LookupEnv(key); exported { + continue + } + } + if err := os.Setenv(key, value); err != nil { + slog.Warn("failed to apply env file variable", "file", envFile, "variable", key, "error", err) + continue + } + d.applied[key] = value + } + + for key := range d.applied { + if _, present := values[key]; present { + continue + } + if err := os.Unsetenv(key); err != nil { + slog.Warn("failed to unset removed env file variable", "file", envFile, "variable", key, "error", err) + continue + } + delete(d.applied, key) + } +} + +// writePIDFile records the running process id so `gomodel --reload` can find +// the gateway to signal. The returned function removes the file again. +func writePIDFile(path string) (func(), error) { + remove := func() {} + path = strings.TrimSpace(path) + if path == "" { + return remove, nil + } + if dir := filepath.Dir(path); dir != "" && dir != "." { + if err := os.MkdirAll(dir, 0o755); err != nil { + return remove, fmt.Errorf("create pid file directory %s: %w", dir, err) + } + } + if err := os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())+"\n"), 0o644); err != nil { + return remove, fmt.Errorf("write pid file %s: %w", path, err) + } + return func() { + // Leave the file alone if it is no longer ours: another instance + // started in the same directory has taken the name over, and removing + // it would leave that one unreachable from --reload. + if pid, err := readPIDFile(path); err == nil && pid != os.Getpid() { + return + } + _ = os.Remove(path) + }, nil +} + +func readPIDFile(path string) (int, error) { + data, err := os.ReadFile(path) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return 0, fmt.Errorf("no pid file at %s: is the gateway running?", path) + } + return 0, fmt.Errorf("read pid file %s: %w", path, err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil || pid <= 0 { + return 0, fmt.Errorf("pid file %s does not contain a process id", path) + } + return pid, nil +} + +// sendReloadSignal implements --reload: it tells the gateway recorded in the +// pid file to re-read its configuration, the way `nginx -s reload` does. The +// running process keeps serving on the current configuration if the new one +// turns out to be invalid. +func sendReloadSignal(stdout io.Writer) error { + result, err := config.Load() + if err != nil { + return fmt.Errorf("load config: %w", err) + } + + path := strings.TrimSpace(result.Config.Server.PIDFile) + if path == "" { + return errors.New("no pid file configured: set server.pid_file (or PID_FILE) on the gateway and on this command") + } + pid, err := readPIDFile(path) + if err != nil { + return err + } + process, err := os.FindProcess(pid) + if err != nil { + return fmt.Errorf("find process %d from %s: %w", pid, path, err) + } + if err := process.Signal(reloadSignal); err != nil { + if errors.Is(err, os.ErrProcessDone) || errors.Is(err, syscall.ESRCH) { + return fmt.Errorf("process %d from %s is not running: remove the stale pid file", pid, path) + } + return fmt.Errorf("signal process %d from %s: %w", pid, path, err) + } + + fmt.Fprintf(stdout, "reload requested (pid %d)\n", pid) + return nil +} diff --git a/run/reload_test.go b/run/reload_test.go new file mode 100644 index 000000000..0fc91c897 --- /dev/null +++ b/run/reload_test.go @@ -0,0 +1,279 @@ +package run + +import ( + "context" + "errors" + "net" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" +) + +// A .env value applies only where the real environment has nothing to say, +// which is godotenv.Load's rule and therefore the rule a reload has to keep. +func TestDotenvLeavesExportedVariablesAlone(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv("GOMODEL_TEST_EXPORTED", "from-environment") + writeEnvFile(t, "GOMODEL_TEST_EXPORTED=from-file\nGOMODEL_TEST_FILE_ONLY=from-file\n") + t.Cleanup(func() { os.Unsetenv("GOMODEL_TEST_FILE_ONLY") }) + + newDotenv().apply() + + if got := os.Getenv("GOMODEL_TEST_EXPORTED"); got != "from-environment" { + t.Errorf("exported variable = %q, want it untouched by the env file", got) + } + if got := os.Getenv("GOMODEL_TEST_FILE_ONLY"); got != "from-file" { + t.Errorf("file-only variable = %q, want %q", got, "from-file") + } +} + +// Reloading is worth little if it cannot see edited credentials and endpoints, +// so a second apply must pick up new values and forget deleted ones — without +// ever taking over a variable the process was started with. +func TestDotenvReappliesEditedFile(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv("GOMODEL_TEST_EXPORTED", "from-environment") + writeEnvFile(t, "GOMODEL_TEST_EXPORTED=from-file\nGOMODEL_TEST_EDITED=before\nGOMODEL_TEST_REMOVED=present\n") + t.Cleanup(func() { + os.Unsetenv("GOMODEL_TEST_EDITED") + os.Unsetenv("GOMODEL_TEST_REMOVED") + }) + + env := newDotenv() + env.apply() + writeEnvFile(t, "GOMODEL_TEST_EXPORTED=from-file\nGOMODEL_TEST_EDITED=after\n") + env.apply() + + if got := os.Getenv("GOMODEL_TEST_EDITED"); got != "after" { + t.Errorf("edited variable = %q, want %q", got, "after") + } + if _, present := os.LookupEnv("GOMODEL_TEST_REMOVED"); present { + t.Error("variable dropped from the env file is still set") + } + if got := os.Getenv("GOMODEL_TEST_EXPORTED"); got != "from-environment" { + t.Errorf("exported variable = %q, want it untouched by the env file", got) + } +} + +// A missing .env file is the normal case for container deployments: it means +// "configuration comes from the environment", not "keep the last file I saw". +func TestDotenvClearsWhenTheFileDisappears(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + writeEnvFile(t, "GOMODEL_TEST_VANISHING=present\n") + t.Cleanup(func() { os.Unsetenv("GOMODEL_TEST_VANISHING") }) + + env := newDotenv() + env.apply() + if err := os.Remove(filepath.Join(dir, envFile)); err != nil { + t.Fatal(err) + } + env.apply() + + if _, present := os.LookupEnv("GOMODEL_TEST_VANISHING"); present { + t.Error("variable survived the removal of the env file") + } +} + +func TestPIDFileRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "gomodel.pid") + + remove, err := writePIDFile(path) + if err != nil { + t.Fatalf("writePIDFile() error = %v", err) + } + pid, err := readPIDFile(path) + if err != nil { + t.Fatalf("readPIDFile() error = %v", err) + } + if pid != os.Getpid() { + t.Errorf("pid = %d, want %d", pid, os.Getpid()) + } + + remove() + if _, err := readPIDFile(path); err == nil { + t.Error("readPIDFile() after removal = nil error, want an error") + } +} + +func TestPIDFileEmptyPathIsANoop(t *testing.T) { + remove, err := writePIDFile(" ") + if err != nil { + t.Fatalf("writePIDFile(\"\") error = %v", err) + } + remove() +} + +func TestReadPIDFileRejectsGarbage(t *testing.T) { + path := filepath.Join(t.TempDir(), "gomodel.pid") + if err := os.WriteFile(path, []byte("not-a-pid"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := readPIDFile(path); err == nil { + t.Error("readPIDFile() on a garbage file = nil error, want an error") + } +} + +// The whole point of building the replacement before stopping what is running: +// a configuration that does not load must cost nothing but a log line. +func TestServeUntilShutdownKeepsServingWhenReloadFails(t *testing.T) { + socket := testSocket(t) + first := newFakeGeneration() + second := newFakeGeneration() + + attempts := make(chan struct{}, 2) + var count atomic.Int32 + rebuild := func() (lifecycleApp, error) { + defer func() { attempts <- struct{}{} }() + if count.Add(1) == 1 { + return nil, errors.New("invalid configuration") + } + return second, nil + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + reload := make(chan os.Signal, 1) + served := make(chan error, 1) + go func() { served <- serveUntilShutdown(ctx, reload, socket, first, rebuild) }() + + <-first.started + reload <- reloadSignal + <-attempts + if first.shutdowns.Load() != 0 { + t.Fatal("a failed reload stopped the running generation") + } + + reload <- reloadSignal + <-attempts + <-second.started + if got := first.shutdowns.Load(); got != 1 { + t.Fatalf("first generation shutdowns = %d, want 1", got) + } + + cancel() + select { + case err := <-served: + if err != nil { + t.Fatalf("serveUntilShutdown() error = %v, want nil", err) + } + case <-time.After(5 * time.Second): + t.Fatal("serveUntilShutdown did not return after cancellation") + } + if got := second.shutdowns.Load(); got != 1 { + t.Fatalf("second generation shutdowns = %d, want 1", got) + } +} + +// Reloading must not cost the port: the socket outlives every generation, so a +// client connecting mid-swap waits in the accept queue instead of being +// refused. +func TestBoundSocketSurvivesGenerations(t *testing.T) { + socket := testSocket(t) + + first, err := socket.next() + if err != nil { + t.Fatalf("socket.next() error = %v", err) + } + address := first.Addr().String() + if err := first.Close(); err != nil { + t.Fatalf("close first listener: %v", err) + } + + second, err := socket.next() + if err != nil { + t.Fatalf("socket.next() after a generation ended = %v", err) + } + defer second.Close() + if got := second.Addr().String(); got != address { + t.Errorf("second generation address = %q, want %q", got, address) + } + + accepted := make(chan error, 1) + go func() { + conn, err := second.Accept() + if err == nil { + _ = conn.Close() + } + accepted <- err + }() + conn, err := net.DialTimeout("tcp", address, 2*time.Second) + if err != nil { + t.Fatalf("dial the reloaded gateway: %v", err) + } + defer conn.Close() + if err := <-accepted; err != nil { + t.Fatalf("accept on the reloaded gateway: %v", err) + } +} + +func testSocket(t *testing.T) *boundSocket { + t.Helper() + socket, err := listenOn("127.0.0.1:0") + if err != nil { + t.Fatalf("listenOn() error = %v", err) + } + t.Cleanup(func() { _ = socket.Close() }) + return socket +} + +func writeEnvFile(t *testing.T, contents string) { + t.Helper() + if err := os.WriteFile(envFile, []byte(contents), 0o600); err != nil { + t.Fatal(err) + } +} + +// fakeGeneration stands in for one built application: it serves until it is +// shut down, and records how often that happened. +type fakeGeneration struct { + started chan struct{} + stopped chan struct{} + stopOnce sync.Once + shutdowns atomic.Int32 +} + +func newFakeGeneration() *fakeGeneration { + return &fakeGeneration{started: make(chan struct{}), stopped: make(chan struct{})} +} + +func (g *fakeGeneration) StartWithListener(_ context.Context, listener net.Listener) error { + if listener != nil { + defer listener.Close() + } + close(g.started) + <-g.stopped + return nil +} + +func (g *fakeGeneration) Shutdown(context.Context) error { + g.shutdowns.Add(1) + g.stopOnce.Do(func() { close(g.stopped) }) + return nil +} + +// A second instance started in the same directory owns the pid file; the first +// one must not remove it on its way out, or --reload loses the survivor. +func TestPIDFileRemovalLeavesAnotherInstanceAlone(t *testing.T) { + path := filepath.Join(t.TempDir(), "gomodel.pid") + remove, err := writePIDFile(path) + if err != nil { + t.Fatalf("writePIDFile() error = %v", err) + } + if err := os.WriteFile(path, []byte("424242\n"), 0o644); err != nil { + t.Fatal(err) + } + + remove() + + pid, err := readPIDFile(path) + if err != nil { + t.Fatalf("readPIDFile() error = %v, want the other instance's pid file intact", err) + } + if pid != 424242 { + t.Errorf("pid = %d, want 424242", pid) + } +} diff --git a/run/run.go b/run/run.go index 275a2f14d..4358a3fed 100644 --- a/run/run.go +++ b/run/run.go @@ -18,14 +18,13 @@ import ( "fmt" "io" "log/slog" + "net" "os" "os/signal" "runtime" "syscall" "time" - "github.com/joho/godotenv" - "github.com/enterpilot/gomodel/config" "github.com/enterpilot/gomodel/ext" "github.com/enterpilot/gomodel/internal/app" @@ -101,11 +100,16 @@ func ExitCode(err error) int { return 1 } -// Run executes the full gateway lifecycle: CLI parsing, --version and -// --health/--ready probe modes, dotenv loading, logging setup, config -// loading, provider registration, application construction (including -// registered extensions), signal handling, and start with graceful shutdown. -// Cancelling ctx triggers the same graceful shutdown as SIGINT/SIGTERM. +// Run executes the full gateway lifecycle: CLI parsing, --version, +// --health/--ready probe and --reload signalling modes, dotenv loading, +// logging setup, config loading, provider registration, application +// construction (including registered extensions), signal handling, and start +// with graceful shutdown. +// +// Cancelling ctx triggers the same graceful shutdown as SIGINT/SIGTERM. A +// reload signal (SIGHUP, what `gomodel --reload` sends) instead re-reads the +// environment file and the configuration and replaces the running application +// with one built from them, without giving up the listening socket. func Run(ctx context.Context, opts Options) error { opts = opts.withDefaults() @@ -122,7 +126,8 @@ func Run(ctx context.Context, opts Options) error { return nil } - _ = godotenv.Load() + env := newDotenv() + env.apply() if cliOpts.Health { if err := runHealthProbe(cliOpts.HealthTimeout); err != nil { @@ -140,6 +145,14 @@ func Run(ctx context.Context, opts Options) error { return nil } + if cliOpts.Reload { + if err := sendReloadSignal(opts.Stdout); err != nil { + fmt.Fprintf(opts.Stderr, "reload failed: %v\n", err) + return err + } + return nil + } + demoMode, err := demoModeFromEnv() if err != nil { fmt.Fprintln(opts.Stderr, err) @@ -169,29 +182,73 @@ func Run(ctx context.Context, opts Options) error { } } - result, err := config.Load() + // build produces one generation of the gateway from the configuration as it + // stands right now. It is called again for every reload, which is what makes + // a reload equivalent to a restart in effect: every configuration value is + // re-read, not a hand-picked subset. + build := func() (*app.App, *config.Config, error) { + result, err := config.Load() + if err != nil { + return nil, nil, fmt.Errorf("failed to load config: %w", err) + } + opts.ConfigureSwaggerDocs(result.Config.Server.BasePath) + + application, err := app.New(ctx, app.Config{ + AppConfig: result, + Factory: defaultProviderFactory(result.Config), + Extensions: opts.Extensions, + DemoMode: demoMode, + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to initialize application: %w", err) + } + return application, result.Config, nil + } + + application, appCfg, err := build() if err != nil { - slog.Error("failed to load config", "error", err) + slog.Error("startup failed", "error", err) return err } - opts.ConfigureSwaggerDocs(result.Config.Server.BasePath) - - application, err := app.New(ctx, app.Config{ - AppConfig: result, - Factory: defaultProviderFactory(result.Config), - Extensions: opts.Extensions, - DemoMode: demoMode, - }) + + socket, err := listenOn(":" + appCfg.Server.Port) if err != nil { - slog.Error("failed to initialize application", "error", err) + slog.Error("failed to bind the server address", "error", err) + _ = shutdownApplicationWithTimeout(application) return err } + defer func() { _ = socket.Close() }() + + removePIDFile, err := writePIDFile(appCfg.Server.PIDFile) + if err != nil { + slog.Warn("could not write the pid file; --reload will not find this instance", "error", err) + } + defer removePIDFile() signalCtx, stop := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM) defer stop() - addr := ":" + result.Config.Server.Port - if err := serveUntilShutdown(signalCtx, application, addr); err != nil { + reload := make(chan os.Signal, 1) + signal.Notify(reload, reloadSignal) + defer signal.Stop(reload) + + rebuild := func() (lifecycleApp, error) { + // The environment file is re-read first so config.Load sees the new + // values; variables exported into the process keep winning over it. + env.apply() + if err := configureLogging(opts.Stderr); err != nil { + return nil, err + } + next, nextCfg, err := build() + if err != nil { + return nil, err + } + warnAboutStartupOnlySettings(appCfg.Server, nextCfg.Server) + appCfg = nextCfg + return next, nil + } + + if err := serveUntilShutdown(signalCtx, reload, socket, application, rebuild); err != nil { slog.Error("application failed", "error", err) return err } @@ -204,12 +261,78 @@ func versionLine(productName string) string { } type lifecycleApp interface { - Start(ctx context.Context, addr string) error + StartWithListener(ctx context.Context, listener net.Listener) error Shutdown(ctx context.Context) error } -// serveUntilShutdown starts the application and returns only once the server -// has stopped *and* the teardown that stopped it has finished. +// serveUntilShutdown serves the gateway until it is asked to stop, replacing +// the running application with a freshly configured one every time a reload +// signal arrives. +// +// A reload builds its replacement before stopping the generation it replaces, +// so a configuration that fails to load or to initialize leaves the gateway +// serving on the one that already works — nginx's rule, and the reason the +// operator can reload without holding their breath. +func serveUntilShutdown(ctx context.Context, reload <-chan os.Signal, socket *boundSocket, application lifecycleApp, rebuild func() (lifecycleApp, error)) error { + for { + listener, err := socket.next() + if err != nil { + _ = shutdownApplicationWithTimeout(application) + return err + } + + generationCtx, endGeneration := context.WithCancel(ctx) + replacement := watchForReload(generationCtx, endGeneration, reload, rebuild) + + startErr := serveGeneration(generationCtx, application, listener) + endGeneration() + + next := <-replacement + switch { + case next == nil: + return startErr + case startErr != nil || ctx.Err() != nil: + // The gateway is on its way out anyway, so the replacement built + // alongside the shutdown never gets to serve. + _ = shutdownApplicationWithTimeout(next) + return startErr + } + application = next + slog.Info("configuration reloaded") + } +} + +// watchForReload turns reload signals into the next application generation. It +// builds the replacement first and ends the running generation only once that +// succeeded, so a failed build is a logged non-event rather than an outage. The +// returned channel yields the replacement, or nil when the generation ended for +// any other reason. +func watchForReload(ctx context.Context, endGeneration context.CancelFunc, reload <-chan os.Signal, rebuild func() (lifecycleApp, error)) <-chan lifecycleApp { + replacement := make(chan lifecycleApp, 1) + go func() { + defer close(replacement) + for { + select { + case <-ctx.Done(): + return + case <-reload: + slog.Info("reloading configuration") + next, err := rebuild() + if err != nil { + slog.Error("reload failed; keeping the running configuration", "error", err) + continue + } + replacement <- next + endGeneration() + return + } + } + }() + return replacement +} + +// serveGeneration starts one application generation and returns only once its +// server has stopped *and* the teardown that stopped it has finished. // // The teardown has to run on its own goroutine because Start blocks until the // server stops and Shutdown is what stops it. Waiting for that goroutine here @@ -218,12 +341,12 @@ type lifecycleApp interface { // records, the database handle — would be dropped on every Ctrl+C. // // This is the only caller of shutdownApplication, so teardown runs exactly -// once per exit and on a single shutdownTimeout budget, whichever way the -// server ended: a signal, a stop of its own accord, or a Start that never got -// off the ground all converge here. Routing the failed-start path through the -// same place is what removes the second teardown that used to run alongside -// it, and with it any reliance on Shutdown being idempotent. -func serveUntilShutdown(ctx context.Context, application lifecycleApp, addr string) error { +// once per generation and on a single shutdownTimeout budget, whichever way the +// server ended: a signal, a reload, a stop of its own accord, or a Start that +// never got off the ground all converge here. Routing the failed-start path +// through the same place is what removes the second teardown that used to run +// alongside it, and with it any reliance on Shutdown being idempotent. +func serveGeneration(ctx context.Context, application lifecycleApp, listener net.Listener) error { serverReturned := make(chan struct{}) shutdownDone := make(chan error, 1) go func() { @@ -231,12 +354,10 @@ func serveUntilShutdown(ctx context.Context, application lifecycleApp, addr stri case <-ctx.Done(): case <-serverReturned: } - shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) - defer cancel() - shutdownDone <- shutdownApplication(application, shutdownCtx) + shutdownDone <- shutdownApplicationWithTimeout(application) }() - startErr := application.Start(context.Background(), addr) + startErr := application.StartWithListener(context.Background(), listener) close(serverReturned) if err := <-shutdownDone; err != nil { @@ -245,6 +366,26 @@ func serveUntilShutdown(ctx context.Context, application lifecycleApp, addr stri return startErr } +func shutdownApplicationWithTimeout(application lifecycleApp) error { + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + return shutdownApplication(application, shutdownCtx) +} + +// warnAboutStartupOnlySettings reports the settings a reload cannot apply: the +// listening socket is kept bound across generations precisely so no connection +// is refused, and the pid file names the process that is already running. +func warnAboutStartupOnlySettings(current, next config.ServerConfig) { + if next.Port != current.Port { + slog.Warn("server port change needs a restart; keeping the bound address", + "bound_port", current.Port, "configured_port", next.Port) + } + if next.PIDFile != current.PIDFile { + slog.Warn("pid file change needs a restart; keeping the current pid file", + "current_pid_file", current.PIDFile, "configured_pid_file", next.PIDFile) + } +} + func shutdownApplication(application lifecycleApp, ctx context.Context) error { done := make(chan error, 1) go func() { diff --git a/run/socket.go b/run/socket.go new file mode 100644 index 000000000..ea2fea3f3 --- /dev/null +++ b/run/socket.go @@ -0,0 +1,76 @@ +package run + +import ( + "fmt" + "log/slog" + "net" + "os" +) + +// boundSocket owns the gateway's listening socket for the lifetime of the +// process so that a configuration reload never gives the port up. +// +// Each generation of the application is handed its own listener and closes it +// when it stops; the socket itself survives because boundSocket holds a +// duplicate descriptor and is the last to let go. Connections that arrive +// while one generation is draining and the next is starting therefore wait in +// the kernel's accept queue instead of being refused. +// +// Windows cannot duplicate a listening socket. There the plain listener is +// served directly and a later generation rebinds the address — reload is a +// POSIX-signal feature, so that path only matters if a rebind is ever needed. +type boundSocket struct { + address string + file *os.File + listener net.Listener +} + +func listenOn(address string) (*boundSocket, error) { + listener, err := net.Listen("tcp", address) + if err != nil { + return nil, err + } + + tcp, ok := listener.(*net.TCPListener) + if !ok { + return &boundSocket{address: address, listener: listener}, nil + } + file, err := tcp.File() + if err != nil { + slog.Debug("listening socket cannot be duplicated; a reload will rebind the address", + "address", address, "error", err) + return &boundSocket{address: address, listener: listener}, nil + } + // The socket stays open through the duplicate held in file. + _ = listener.Close() + return &boundSocket{address: address, file: file}, nil +} + +// next returns the listener for the next application generation. +func (s *boundSocket) next() (net.Listener, error) { + if s.file != nil { + listener, err := net.FileListener(s.file) + if err != nil { + return nil, fmt.Errorf("reuse listening socket on %s: %w", s.address, err) + } + return listener, nil + } + if s.listener != nil { + listener := s.listener + s.listener = nil + return listener, nil + } + return net.Listen("tcp", s.address) +} + +func (s *boundSocket) Close() error { + if s.listener != nil { + defer func() { s.listener = nil }() + return s.listener.Close() + } + if s.file != nil { + defer func() { s.file = nil }() + return s.file.Close() + } + return nil +} From 84c1add8e8a565df2bc0ac60fe88e9777a70c0c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 21:59:36 +0000 Subject: [PATCH 2/4] fix(server): keep the gateway server configuration on the listener path Serving a pre-bound listener went through a bare Echo start config, so the switch to that path for reload support silently dropped the inbound read/header/write timeouts and the gateway's own graceful drain window from every request. Both start paths now build the same start config, and a test covers the listener one. Also corrects comments that no longer described the code: teardown has more than one entry point now, the pid file is only removed while it is still ours, and current Go can duplicate a listening socket on Windows, so the fallback is stated as a platform capability rather than an assertion about Windows. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty --- internal/server/http.go | 22 ++++++++++++----- internal/server/http_start_test.go | 39 ++++++++++++++++++++++++++++++ run/reload.go | 9 ++++--- run/run.go | 26 +++++++++++--------- run/socket.go | 8 +++--- 5 files changed, 79 insertions(+), 25 deletions(-) diff --git a/internal/server/http.go b/internal/server/http.go index 818427fd1..0cd06b038 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -538,14 +538,16 @@ func (s *Server) Start(ctx context.Context, addr string) error { return newGatewayStartConfig(addr).Start(ctx, s.echo) } -// StartWithListener starts the HTTP server using a pre-bound listener. -// This is useful in tests that need an already-reserved loopback port. +// StartWithListener starts the HTTP server using a pre-bound listener. The +// gateway serves this way in production — the listening socket outlives each +// configuration a reload installs — and tests use it to reserve a loopback +// port up front, so it configures the server exactly like Start does: same +// inbound timeouts, same drain window. func (s *Server) StartWithListener(ctx context.Context, listener net.Listener) error { - sc := echo.StartConfig{ - HideBanner: true, - Listener: listener, + if listener == nil { + return errors.New("listener is required") } - return sc.Start(ctx, s.echo) + return newGatewayStartConfigForListener(listener).Start(ctx, s.echo) } // Shutdown releases server resources. The HTTP server itself is stopped by @@ -606,6 +608,14 @@ func newGatewayStartConfig(addr string) echo.StartConfig { } } +// newGatewayStartConfig with a pre-bound listener. Echo ignores Address once +// Listener is set; it is filled in anyway so the two describe the same server. +func newGatewayStartConfigForListener(listener net.Listener) echo.StartConfig { + sc := newGatewayStartConfig(listener.Addr().String()) + sc.Listener = listener + return sc +} + func configureGatewayHTTPServer(server *http.Server) error { if server == nil { return nil diff --git a/internal/server/http_start_test.go b/internal/server/http_start_test.go index f59cf79ec..04b2ab146 100644 --- a/internal/server/http_start_test.go +++ b/internal/server/http_start_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "net" "net/http" "net/http/httptest" "testing" @@ -120,3 +121,41 @@ func (w *deadlineTrackingWriter) SetWriteDeadline(deadline time.Time) error { w.deadlines = append(w.deadlines, deadline) return nil } + +// The gateway serves on a pre-bound listener in production, because the +// listening socket has to outlive the configuration a reload replaces. That +// path went through a bare start config once, which silently dropped the +// inbound timeouts and the drain window from every request the gateway served. +func TestNewGatewayStartConfigForListener_KeepsTheServerConfiguration(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer listener.Close() + + cfg := newGatewayStartConfigForListener(listener) + + if cfg.Listener != listener { + t.Error("Listener = nil, want the pre-bound listener") + } + if cfg.GracefulTimeout != GracefulDrainTimeout { + t.Errorf("GracefulTimeout = %v, want %v", cfg.GracefulTimeout, GracefulDrainTimeout) + } + if cfg.OnShutdownError == nil { + t.Error("OnShutdownError = nil, want the drain cutoff reported by the gateway") + } + if cfg.BeforeServeFunc == nil { + t.Fatal("BeforeServeFunc = nil, want the inbound server timeouts") + } + + server := &http.Server{} + if err := cfg.BeforeServeFunc(server); err != nil { + t.Fatalf("BeforeServeFunc() error = %v", err) + } + if server.ReadHeaderTimeout != inboundServerReadHeaderTimeout { + t.Errorf("ReadHeaderTimeout = %v, want %v", server.ReadHeaderTimeout, inboundServerReadHeaderTimeout) + } + if server.WriteTimeout != inboundServerWriteTimeout { + t.Errorf("WriteTimeout = %v, want %v", server.WriteTimeout, inboundServerWriteTimeout) + } +} diff --git a/run/reload.go b/run/reload.go index e56d762f3..ef24a8348 100644 --- a/run/reload.go +++ b/run/reload.go @@ -77,7 +77,8 @@ func (d *dotenv) apply() { } // writePIDFile records the running process id so `gomodel --reload` can find -// the gateway to signal. The returned function removes the file again. +// the gateway to signal. The returned function removes the file again, unless +// another instance has claimed it in the meantime. func writePIDFile(path string) (func(), error) { remove := func() {} path = strings.TrimSpace(path) @@ -93,9 +94,9 @@ func writePIDFile(path string) (func(), error) { return remove, fmt.Errorf("write pid file %s: %w", path, err) } return func() { - // Leave the file alone if it is no longer ours: another instance - // started in the same directory has taken the name over, and removing - // it would leave that one unreachable from --reload. + // Leave the file alone if it is no longer ours: another instance has + // taken this path over, and removing it would leave that one + // unreachable from --reload. if pid, err := readPIDFile(path); err == nil && pid != os.Getpid() { return } diff --git a/run/run.go b/run/run.go index 4358a3fed..a9cf5f150 100644 --- a/run/run.go +++ b/run/run.go @@ -304,7 +304,7 @@ func serveUntilShutdown(ctx context.Context, reload <-chan os.Signal, socket *bo // watchForReload turns reload signals into the next application generation. It // builds the replacement first and ends the running generation only once that -// succeeded, so a failed build is a logged non-event rather than an outage. The +// succeeded, so a failed build costs a log line rather than an outage. The // returned channel yields the replacement, or nil when the generation ended for // any other reason. func watchForReload(ctx context.Context, endGeneration context.CancelFunc, reload <-chan os.Signal, rebuild func() (lifecycleApp, error)) <-chan lifecycleApp { @@ -334,18 +334,20 @@ func watchForReload(ctx context.Context, endGeneration context.CancelFunc, reloa // serveGeneration starts one application generation and returns only once its // server has stopped *and* the teardown that stopped it has finished. // -// The teardown has to run on its own goroutine because Start blocks until the -// server stops and Shutdown is what stops it. Waiting for that goroutine here -// is the load-bearing part: the process exits the moment Run returns, so -// anything Shutdown had not reached yet — the buffered usage and audit -// records, the database handle — would be dropped on every Ctrl+C. +// The teardown has to run on its own goroutine because StartWithListener +// blocks until the server stops and Shutdown is what stops it. Waiting for that +// goroutine here is the load-bearing part: the process exits the moment Run +// returns, so anything Shutdown had not reached yet — the buffered usage and +// audit records, the database handle — would be dropped on every Ctrl+C. // -// This is the only caller of shutdownApplication, so teardown runs exactly -// once per generation and on a single shutdownTimeout budget, whichever way the -// server ended: a signal, a reload, a stop of its own accord, or a Start that -// never got off the ground all converge here. Routing the failed-start path -// through the same place is what removes the second teardown that used to run -// alongside it, and with it any reliance on Shutdown being idempotent. +// Every generation that serves is torn down here, exactly once and on a single +// shutdownTimeout budget, whichever way the server ended: a signal, a reload, a +// stop of its own accord, or a start that never got off the ground all converge +// here. Routing the failed-start path through the same place is what removes +// the second teardown that used to run alongside it, and with it any reliance +// on Shutdown being idempotent. An application built but never served — one +// that could not be given a listener, or a replacement overtaken by shutdown — +// is torn down by its owner in serveUntilShutdown, on the same budget. func serveGeneration(ctx context.Context, application lifecycleApp, listener net.Listener) error { serverReturned := make(chan struct{}) shutdownDone := make(chan error, 1) diff --git a/run/socket.go b/run/socket.go index ea2fea3f3..0bd2af0d6 100644 --- a/run/socket.go +++ b/run/socket.go @@ -16,9 +16,11 @@ import ( // while one generation is draining and the next is starting therefore wait in // the kernel's accept queue instead of being refused. // -// Windows cannot duplicate a listening socket. There the plain listener is -// served directly and a later generation rebinds the address — reload is a -// POSIX-signal feature, so that path only matters if a rebind is ever needed. +// Not every platform lets a listening socket be duplicated. Where it cannot be, +// the listener is served directly and a later generation rebinds the address, +// which does leave a gap where connections are refused. Reload is delivered by +// a POSIX signal, so on a platform without one — Windows — that fallback is +// what serves, and there is never a second generation to rebind for. type boundSocket struct { address string file *os.File From 00628434c753a4d8ce0c18582484f6ac8852270f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 22:21:55 +0000 Subject: [PATCH 3/4] docs(cli): correct reload comments and cover the swap window in tests Second pass over the comments added with --reload: - the socket fallback comment welded two independent facts together (descriptor duplication and signal delivery); Windows is out because Go delivers no SIGHUP there, not because the duplicate fails - "a reload equivalent to a restart" overstated it: the socket and the pid file are fixed for the life of the process - the env file comment did not say what happens to an unparsable file (the environment is left as it stands) - the pid file default follows the database to keep state together, not because --reload would otherwise look in the wrong place: both sides resolve the same default either way - two test stubs still described a Start method that is now StartWithListener TestBoundSocketSurvivesGenerations claimed connections wait in the accept queue during a swap but connected after the next listener existed, so it never entered that window. It now connects while nothing is accepting and accepts afterwards, which is the property the design exists for, and skips where the socket cannot be duplicated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty --- config/server_test.go | 8 ++++---- docs/advanced/cli.mdx | 13 ++++++++----- run/lifecycle_test.go | 10 +++++----- run/reload.go | 4 +++- run/reload_test.go | 43 ++++++++++++++++++++++++------------------- run/run.go | 7 ++++--- run/socket.go | 11 ++++++----- 7 files changed, 54 insertions(+), 42 deletions(-) diff --git a/config/server_test.go b/config/server_test.go index b7dfaae51..6dd7f1ad4 100644 --- a/config/server_test.go +++ b/config/server_test.go @@ -8,10 +8,10 @@ import ( "github.com/enterpilot/gomodel/internal/platformdir" ) -// The pid file has to land next to the database, or `gomodel --reload` looks -// for it somewhere the gateway never wrote it: a Docker image with /app/data -// keeps both project-local, a binary install keeps both in the per-user data -// directory. +// The pid file follows the database instead of scattering GoModel's state +// across the filesystem: a Docker image with /app/data keeps both +// project-local, and a binary install started from an arbitrary working +// directory keeps both in the per-user data directory. func TestDefaultPIDFilePath(t *testing.T) { platformDataDir, err := platformdir.DataDir() if err != nil { diff --git a/docs/advanced/cli.mdx b/docs/advanced/cli.mdx index 1f4b1e4f5..b9b7d55a2 100644 --- a/docs/advanced/cli.mdx +++ b/docs/advanced/cli.mdx @@ -100,7 +100,10 @@ restarting it — the same operation as `nginx -s reload`: gomodel --reload ``` -It reads the pid file, signals that process, and exits. The gateway then: +It loads the same configuration the gateway does to find the pid file, signals +that process, and exits. Loading it first means a `config.yaml` the binary +cannot parse fails the command — non-zero exit, no signal sent — before the +running gateway is ever asked to look at it. On success the gateway: 1. Re-reads the `.env` file. New and edited values are applied; values removed from the file are unset. Variables exported into the process environment @@ -119,10 +122,10 @@ already works. The listening socket is held for the lifetime of the process and handed to each configuration in turn, so requests arriving mid-reload wait to be accepted -rather than being refused. In-flight requests are drained first (see -`GracefulDrainTimeout`), and streamed responses that outlive the drain window -are cut, so reload during heavy streaming traffic is not free — but no -connection is dropped at the socket. +rather than being refused. In-flight requests get the same 10-second drain +window as a shutdown, and streamed responses that outlive it are cut, so a +reload during heavy streaming traffic is not free — but no connection is +dropped at the socket. Sending the signal directly does the same thing, which is what a process manager or a container without a shell can use: diff --git a/run/lifecycle_test.go b/run/lifecycle_test.go index 26bf2c598..a787faf63 100644 --- a/run/lifecycle_test.go +++ b/run/lifecycle_test.go @@ -152,12 +152,12 @@ func TestGracefulDrainFitsInsideTheShutdownBudget(t *testing.T) { } } -// servingApp mirrors the ordering that matters in the real App: Start blocks -// until Shutdown stops the server, and Shutdown keeps working afterwards — -// flushing buffered usage and audit records, closing the database — before it -// returns. +// servingApp mirrors the ordering that matters in the real App: +// StartWithListener blocks until Shutdown stops the server, and Shutdown keeps +// working afterwards — flushing buffered usage and audit records, closing the +// database — before it returns. type servingApp struct { - serverStopped chan struct{} // closed by Shutdown, releases Start + serverStopped chan struct{} // closed by Shutdown, releases StartWithListener flushing chan struct{} // closed by the test, releases Shutdown shutdownDone atomic.Bool } diff --git a/run/reload.go b/run/reload.go index ef24a8348..d389560ba 100644 --- a/run/reload.go +++ b/run/reload.go @@ -40,7 +40,9 @@ func newDotenv() *dotenv { // apply merges the current contents of envFile into the process environment. // A missing file is normal — configuration may come entirely from the real -// environment — and clears whatever the file previously contributed. +// environment — and clears whatever the file previously contributed. A file +// that cannot be read or parsed is not: it leaves the environment as it stands, +// so a half-typed edit does not strip the running configuration. func (d *dotenv) apply() { values, err := godotenv.Read(envFile) if err != nil { diff --git a/run/reload_test.go b/run/reload_test.go index 0fc91c897..16423fe27 100644 --- a/run/reload_test.go +++ b/run/reload_test.go @@ -168,11 +168,15 @@ func TestServeUntilShutdownKeepsServingWhenReloadFails(t *testing.T) { } } -// Reloading must not cost the port: the socket outlives every generation, so a -// client connecting mid-swap waits in the accept queue instead of being -// refused. +// Reloading must not cost the port. This walks the window a reload opens: the +// generation that was serving has closed its listener and the next one has not +// started, and a client connecting right then must still be connected — waiting +// in the kernel's accept queue — rather than refused. func TestBoundSocketSurvivesGenerations(t *testing.T) { socket := testSocket(t) + if socket.file == nil { + t.Skip("this platform cannot duplicate the listening socket; generations rebind instead") + } first, err := socket.next() if err != nil { @@ -183,31 +187,32 @@ func TestBoundSocketSurvivesGenerations(t *testing.T) { t.Fatalf("close first listener: %v", err) } + // Nothing is accepting at this point. + conn, err := net.DialTimeout("tcp", address, 5*time.Second) + if err != nil { + t.Fatalf("connect while no generation is accepting: %v", err) + } + defer conn.Close() + second, err := socket.next() if err != nil { t.Fatalf("socket.next() after a generation ended = %v", err) } defer second.Close() if got := second.Addr().String(); got != address { - t.Errorf("second generation address = %q, want %q", got, address) + t.Fatalf("second generation address = %q, want %q", got, address) } - - accepted := make(chan error, 1) - go func() { - conn, err := second.Accept() - if err == nil { - _ = conn.Close() + if deadliner, ok := second.(interface{ SetDeadline(time.Time) error }); ok { + if err := deadliner.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { + t.Fatalf("set accept deadline: %v", err) } - accepted <- err - }() - conn, err := net.DialTimeout("tcp", address, 2*time.Second) - if err != nil { - t.Fatalf("dial the reloaded gateway: %v", err) } - defer conn.Close() - if err := <-accepted; err != nil { - t.Fatalf("accept on the reloaded gateway: %v", err) + + waiting, err := second.Accept() + if err != nil { + t.Fatalf("accept the connection that waited through the swap: %v", err) } + _ = waiting.Close() } func testSocket(t *testing.T) *boundSocket { @@ -255,7 +260,7 @@ func (g *fakeGeneration) Shutdown(context.Context) error { return nil } -// A second instance started in the same directory owns the pid file; the first +// A second instance configured with the same pid file path owns it; the first // one must not remove it on its way out, or --reload loses the survivor. func TestPIDFileRemovalLeavesAnotherInstanceAlone(t *testing.T) { path := filepath.Join(t.TempDir(), "gomodel.pid") diff --git a/run/run.go b/run/run.go index a9cf5f150..65416c447 100644 --- a/run/run.go +++ b/run/run.go @@ -183,9 +183,10 @@ func Run(ctx context.Context, opts Options) error { } // build produces one generation of the gateway from the configuration as it - // stands right now. It is called again for every reload, which is what makes - // a reload equivalent to a restart in effect: every configuration value is - // re-read, not a hand-picked subset. + // stands right now. It is called again for every reload, which is what lets + // a reload re-read every configuration value rather than a hand-picked + // subset — everything except what is fixed for the life of the process (see + // warnAboutStartupOnlySettings). build := func() (*app.App, *config.Config, error) { result, err := config.Load() if err != nil { diff --git a/run/socket.go b/run/socket.go index 0bd2af0d6..1fb1ff354 100644 --- a/run/socket.go +++ b/run/socket.go @@ -16,11 +16,12 @@ import ( // while one generation is draining and the next is starting therefore wait in // the kernel's accept queue instead of being refused. // -// Not every platform lets a listening socket be duplicated. Where it cannot be, -// the listener is served directly and a later generation rebinds the address, -// which does leave a gap where connections are refused. Reload is delivered by -// a POSIX signal, so on a platform without one — Windows — that fallback is -// what serves, and there is never a second generation to rebind for. +// Duplicating the descriptor is what keeps the socket alive, and not every +// platform and listener type supports it. Where it fails, the listener is +// served directly and a later generation rebinds the address instead, which +// does leave a gap where connections are refused. Windows never reaches that +// gap for a different reason: Go delivers no SIGHUP there, so nothing asks for +// a second generation in the first place. type boundSocket struct { address string file *os.File From 0e35bbd2b10655aefae2d58120f88b175c84e3ac Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 22:47:55 +0000 Subject: [PATCH 4/4] fix(run): keep a rejected reload from changing the running gateway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the PR review findings. A reload has to read the environment file and install the new logging configuration before it can build the replacement, but both are process-wide: when the build then failed, the generation that kept serving was already running under the environment and log level the operator had just been told were rejected. Both are now rolled back, including the dotenv bookkeeping, so a later reload still applies the same file. The reload signal is also claimed before the pid file is written. The pid file is what tells an operator this instance can be signalled, and until Notify runs SIGHUP still carries its default disposition — it would have killed the gateway instead of reloading it. An empty PID_FILE turned out not to disable the pid file: empty env vars read as unset throughout this config, so the default won. The documentation now says what actually disables it (`server.pid_file: ""` in config.yaml) and a test pins all three outcomes. The restart-only nature of pid_file was documented in CLAUDE.md but nowhere an operator would look, so it is now stated with the setting itself, and the no-refused-connections guarantee is qualified with what it depends on. DefaultSQLitePath and DefaultPIDFilePath resolved the ./data-or-per-user directory rule separately; both now call platformdir.DataFile. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty --- .env.template | 6 +- CLAUDE.md | 2 +- config/config.example.yaml | 2 +- config/server.go | 20 ++--- config/server_test.go | 54 ++++++++++-- docs/advanced/cli.mdx | 19 +++-- docs/advanced/config-yaml.mdx | 4 +- docs/advanced/configuration.mdx | 2 +- internal/platformdir/platformdir.go | 26 ++++++ internal/server/http_start_test.go | 25 +++++- internal/storage/storage.go | 13 +-- run/reload.go | 40 ++++++++- run/reload_test.go | 127 ++++++++++++++++++++++++++++ run/run.go | 35 ++++++-- run/socket.go | 3 + 15 files changed, 319 insertions(+), 59 deletions(-) diff --git a/.env.template b/.env.template index 58d420cf9..992c9f293 100644 --- a/.env.template +++ b/.env.template @@ -7,8 +7,10 @@ # Where the running gateway records its process id so `gomodel --reload` can find it # (default: data/gomodel.pid next to a ./data directory, otherwise the per-user data -# directory). Set it per instance when several gateways share a host; set it empty to -# write no pid file, which also disables --reload. +# directory). Set it per instance when several gateways share a host. Leaving this +# empty means "unset" and keeps the default, as everywhere else here; to write no pid +# file at all (which also disables --reload), set `server.pid_file: ""` in config.yaml. +# Changing it takes effect on the next restart, not on `gomodel --reload`. # PID_FILE=data/gomodel.pid # Reject unknown keys in config.yaml and in the JSON env vars that declare the same diff --git a/CLAUDE.md b/CLAUDE.md index 2d0bf1bff..0901a1cb9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,7 +111,7 @@ Full reference: `.env.template` and `config/config.yaml` - `GOMODEL_MASTER_KEY` (empty = unsafe mode). Managed API keys (dashboard API Keys page / `POST /admin/auth-keys`) carry a per-key `dashboard_access` flag (default false, changeable via `PUT /admin/auth-keys/{id}/dashboard-access`): only the master key and flagged keys can call the admin REST API endpoints under `/admin/*` (others get 403 `dashboard_access_denied`); the dashboard UI shell and static assets (`/admin/dashboard`, `/admin/static/*`) skip auth entirely — only the admin data they load is gated; model endpoints and `GET /v1/usage` stay open to every key, and the no-master-key lockout-recovery path (auth skipped on `/admin/*`) is unaffected. - `BODY_SIZE_LIMIT` ("10M") - `USER_PATH_HEADER` (`X-GoModel-User-Path`: Header used to read/write request `user_path` values) - - `PID_FILE` / `server.pid_file` (`data/gomodel.pid` next to a `./data` directory, otherwise the OS per-user data dir — same resolution as `SQLITE_PATH`): where the running gateway records its process id. `gomodel --reload` reads it and signals that process (SIGHUP; `kill -HUP` works too) to reload configuration without a restart, like `nginx -s reload`. The reload re-reads `.env` (exported variables still win over the file; variables removed from the file are unset) and the whole config, then rebuilds the application — so every setting reloads, not a curated subset. The replacement is built before the running one is stopped, so a broken config keeps the current one serving; the listening socket is held across generations, so no connection is refused mid-reload. `PORT` and `PID_FILE` changes still need a restart (warned about), and in-memory state — rate limit counters, session affinity pins, live log buffers — resets as it would on restart. Empty disables the pid file and `--reload`. Not available on Windows (POSIX signals). + - `PID_FILE` / `server.pid_file` (`data/gomodel.pid` next to a `./data` directory, otherwise the OS per-user data dir — same resolution as `SQLITE_PATH`): where the running gateway records its process id. `gomodel --reload` reads it and signals that process (SIGHUP; `kill -HUP` works too) to reload configuration without a restart, like `nginx -s reload`. The reload re-reads `.env` (exported variables still win over the file; variables removed from the file are unset) and the whole config, then rebuilds the application — so every setting reloads, not a curated subset. The replacement is built before the running one is stopped, so a broken config keeps the current one serving; the listening socket is held across generations, so no connection is refused mid-reload. `PORT` and `PID_FILE` changes still need a restart (warned about), and in-memory state — rate limit counters, session affinity pins, live log buffers — resets as it would on restart. `server.pid_file: ""` in `config.yaml` disables the pid file and `--reload` (an empty `PID_FILE` env var reads as unset and keeps the default). Not available on Windows (POSIX signals). - `ENABLE_PASSTHROUGH_ROUTES` (true: Enable provider-native passthrough routes under /p/{provider}/...) - `ALLOW_PASSTHROUGH_V1_ALIAS` (true: Allow /p/{provider}/v1/... aliases while keeping /p/{provider}/... canonical) - `ENABLED_PASSTHROUGH_PROVIDERS` (openai,anthropic,openrouter,zai,vllm: Comma-separated list of enabled passthrough providers) diff --git a/config/config.example.yaml b/config/config.example.yaml index 98e37f85c..9805425a2 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -15,7 +15,7 @@ server: user_path_header: "X-GoModel-User-Path" # env: USER_PATH_HEADER; inbound header used for user_path scoping enabled_passthrough_providers: ["openai", "anthropic", "cohere", "openrouter", "kilo", "zai", "vllm", "deepseek", "bailian"] # providers enabled on /p/{provider}/... realtime_enabled: true # env: REALTIME_ENABLED; expose /v1/realtime websocket and /p/{provider}/v1/realtime upgrades (OpenAI only) - pid_file: "data/gomodel.pid" # env: PID_FILE; where the running gateway records its process id so `gomodel --reload` can find it. Set per instance when several gateways share a host; empty writes no pid file and disables --reload + pid_file: "data/gomodel.pid" # env: PID_FILE; where the running gateway records its process id so `gomodel --reload` can find it. Set per instance when several gateways share a host; empty writes no pid file and disables --reload; changing it needs a restart, not a reload models: enabled_by_default: true # env: MODELS_ENABLED_BY_DEFAULT; when false, models stay unavailable until an access override allows one or more user paths diff --git a/config/server.go b/config/server.go index 200efa955..cabf5370c 100644 --- a/config/server.go +++ b/config/server.go @@ -3,9 +3,7 @@ package config import ( "fmt" "net/textproto" - "os" "path" - "path/filepath" "regexp" "strconv" "strings" @@ -49,8 +47,11 @@ type ServerConfig struct { RealtimeEnabled bool `yaml:"realtime_enabled" env:"REALTIME_ENABLED"` // PIDFile records the process id of the running gateway so `gomodel --reload` // can find it. Default: DefaultPIDFilePath(). Set it per instance when - // several gateways share a host; set it empty to write no pid file, which - // also disables `--reload`. + // several gateways share a host, or to "" in config.yaml to write no pid + // file at all, which also disables `--reload` (an empty PID_FILE reads as + // unset, like every other env var here, and keeps the default). Changing it + // needs a restart — it names the process that is already running — so a + // reload only warns about it. PIDFile string `yaml:"pid_file" env:"PID_FILE"` } @@ -61,16 +62,9 @@ const LegacyPIDFilePath = "data/gomodel.pid" // DefaultPIDFilePath returns the pid file path used when none is configured: // LegacyPIDFilePath when a ./data directory already exists (Docker images and // existing deployments), otherwise the OS-conventional per-user data directory -// — the same resolution storage.DefaultSQLitePath uses for the database. +// — the same resolution the database uses, so both land together. func DefaultPIDFilePath() string { - if info, err := os.Stat("data"); err == nil && info.IsDir() { - return LegacyPIDFilePath - } - dir, err := platformdir.DataDir() - if err != nil { - return LegacyPIDFilePath - } - return filepath.Join(dir, "gomodel.pid") + return platformdir.DataFile("gomodel.pid") } var headerNameRegex = regexp.MustCompile(`^[!#$%&'*+\-.^_` + "`" + `|~0-9A-Za-z]+$`) diff --git a/config/server_test.go b/config/server_test.go index 6dd7f1ad4..262e1adda 100644 --- a/config/server_test.go +++ b/config/server_test.go @@ -52,15 +52,51 @@ func TestDefaultPIDFilePath(t *testing.T) { } } -func TestPIDFileEnvOverride(t *testing.T) { - t.Chdir(t.TempDir()) - t.Setenv("PID_FILE", "/var/run/gomodel/custom.pid") - - result, err := Load() - if err != nil { - t.Fatalf("Load() error = %v", err) +func TestPIDFilePathResolution(t *testing.T) { + tests := []struct { + name string + env string + configYAML string + want string + }{ + { + name: "env var wins", + env: "/var/run/gomodel/custom.pid", + want: "/var/run/gomodel/custom.pid", + }, + { + // Empty env vars are "unset" everywhere in this config, so PID_FILE= + // keeps the default rather than disabling the pid file. Asserted so + // the documented way to disable it stays the config file. + name: "empty env var keeps the default", + env: "", + want: DefaultPIDFilePath(), + }, + { + name: "empty config value writes no pid file", + configYAML: "server:\n pid_file: \"\"\n", + want: "", + }, } - if got := result.Config.Server.PIDFile; got != "/var/run/gomodel/custom.pid" { - t.Errorf("Server.PIDFile = %q, want the PID_FILE value", got) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + t.Setenv("PID_FILE", tt.env) + if tt.configYAML != "" { + if err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(tt.configYAML), 0o600); err != nil { + t.Fatal(err) + } + } + + result, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if got := result.Config.Server.PIDFile; got != tt.want { + t.Errorf("Server.PIDFile = %q, want %q", got, tt.want) + } + }) } } diff --git a/docs/advanced/cli.mdx b/docs/advanced/cli.mdx index b9b7d55a2..e3439cd67 100644 --- a/docs/advanced/cli.mdx +++ b/docs/advanced/cli.mdx @@ -124,8 +124,14 @@ The listening socket is held for the lifetime of the process and handed to each configuration in turn, so requests arriving mid-reload wait to be accepted rather than being refused. In-flight requests get the same 10-second drain window as a shutdown, and streamed responses that outlive it are cut, so a -reload during heavy streaming traffic is not free — but no connection is -dropped at the socket. +reload during heavy streaming traffic is not free — but no connection is dropped +at the socket. + +That last guarantee depends on the operating system letting the gateway +duplicate a listening socket, which covers every platform that can be sent a +reload signal in the first place. Where duplication is unavailable, the next +configuration rebinds the address instead, and connections are refused for the +length of the swap. Sending the signal directly does the same thing, which is what a process manager or a container without a shell can use: @@ -147,10 +153,11 @@ OS-conventional per-user data directory otherwise — the same resolution the SQLite database uses. Give each instance its own path when several gateways share a host, since -`--reload` signals whichever process the file names. Setting it empty writes no -pid file and disables `--reload`; `kill -HUP` still works. If the path is not -writable the gateway logs a warning and serves normally, only without `--reload` -support. +`--reload` signals whichever process the file names. To write no pid file at +all, set `server.pid_file: ""` in `config.yaml` — that disables `--reload`, +though `kill -HUP` still works. An empty `PID_FILE` env var reads as unset and +keeps the default, the same as every other setting. If the path is not writable +the gateway logs a warning and serves normally, only without `--reload` support. Both the gateway and `gomodel --reload` resolve the path from the same configuration, so run the command from the same working directory (or with the diff --git a/docs/advanced/config-yaml.mdx b/docs/advanced/config-yaml.mdx index 80eb9b0f3..9d77740b8 100644 --- a/docs/advanced/config-yaml.mdx +++ b/docs/advanced/config-yaml.mdx @@ -113,7 +113,9 @@ To change the inbound user path header, set `server.user_path_header` or Edits to this file are applied to a running gateway with `gomodel --reload`, which re-reads the file and the environment and rebuilds the gateway in place — see [CLI Operations](/advanced/cli#configuration-reload). Nothing changes if the -new file fails to load. +new file fails to load. `server.port` and `server.pid_file` are the exceptions: +both are fixed for the life of the process, so a reload logs a warning and keeps +the running values until the gateway is restarted. ## Docker diff --git a/docs/advanced/configuration.mdx b/docs/advanced/configuration.mdx index 1a96f8af5..2eb2e0b83 100644 --- a/docs/advanced/configuration.mdx +++ b/docs/advanced/configuration.mdx @@ -47,7 +47,7 @@ The most common way to configure GoModel. Set any of the variables below to over | `GOMODEL_DEMO_MODE` | Enable public demo warnings in logs and the dashboard | `false` | | `BODY_SIZE_LIMIT` | Max request body size (e.g., `10M`, `1024K`, `500KB`) | _(no limit)_ | | `USER_PATH_HEADER` | Header used to read/write request `user_path` values | `X-GoModel-User-Path` | -| `PID_FILE` | Where the running gateway records its process id for `gomodel --reload` | `data/gomodel.pid` next to a `./data` directory, otherwise the per-user data directory | +| `PID_FILE` | Where the running gateway records its process id for `gomodel --reload`. Changing it needs a restart | `data/gomodel.pid` next to a `./data` directory, otherwise the per-user data directory | Set `GOMODEL_DEMO_MODE=true` for a public or shared demonstration instance. GoModel logs a warning at startup and every five minutes, renders a persistent diff --git a/internal/platformdir/platformdir.go b/internal/platformdir/platformdir.go index a2a6c390c..f94002ce4 100644 --- a/internal/platformdir/platformdir.go +++ b/internal/platformdir/platformdir.go @@ -45,6 +45,32 @@ func DataDir() (string, error) { } } +// LocalDataDir is the project-local data directory. Deployments that already +// have one — the Docker image, anyone running from a checkout — keep their +// state there, so upgrades never move a database or a pid file out from under +// an operator. +const LocalDataDir = "data" + +// DataFile returns where a durable data file called name belongs: inside +// LocalDataDir when that directory exists next to the process, otherwise inside +// DataDir. Callers share this rule so a deployment's files stay together +// instead of some landing project-local and others in the per-user directory. +// +// The local form is spelled with a forward slash on every platform, which +// Windows accepts, so it stays comparable to the legacy path constants built +// the same way. +func DataFile(name string) string { + local := LocalDataDir + "/" + name + if info, err := os.Stat(LocalDataDir); err == nil && info.IsDir() { + return local + } + dir, err := DataDir() + if err != nil { + return local + } + return filepath.Join(dir, name) +} + // CacheDir returns the directory for re-creatable caches such as the model // catalog: // diff --git a/internal/server/http_start_test.go b/internal/server/http_start_test.go index 04b2ab146..d6f34654b 100644 --- a/internal/server/http_start_test.go +++ b/internal/server/http_start_test.go @@ -152,10 +152,27 @@ func TestNewGatewayStartConfigForListener_KeepsTheServerConfiguration(t *testing if err := cfg.BeforeServeFunc(server); err != nil { t.Fatalf("BeforeServeFunc() error = %v", err) } - if server.ReadHeaderTimeout != inboundServerReadHeaderTimeout { - t.Errorf("ReadHeaderTimeout = %v, want %v", server.ReadHeaderTimeout, inboundServerReadHeaderTimeout) + for _, timeout := range []struct { + name string + got time.Duration + want time.Duration + }{ + {"ReadTimeout", server.ReadTimeout, inboundServerReadTimeout}, + {"ReadHeaderTimeout", server.ReadHeaderTimeout, inboundServerReadHeaderTimeout}, + {"WriteTimeout", server.WriteTimeout, inboundServerWriteTimeout}, + } { + if timeout.got != timeout.want { + t.Errorf("%s = %v, want %v", timeout.name, timeout.got, timeout.want) + } } - if server.WriteTimeout != inboundServerWriteTimeout { - t.Errorf("WriteTimeout = %v, want %v", server.WriteTimeout, inboundServerWriteTimeout) +} + +// A nil listener is a caller mistake, not something to hand to Echo: it would +// bind a fresh address from the empty start config and serve there instead. +func TestStartWithListenerRejectsANilListener(t *testing.T) { + srv := New(nil, &Config{}) + + if err := srv.StartWithListener(context.Background(), nil); err == nil { + t.Fatal("StartWithListener(nil) error = nil, want an error") } } diff --git a/internal/storage/storage.go b/internal/storage/storage.go index b23c9378c..63c829c6e 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -7,8 +7,6 @@ import ( "context" "database/sql" "fmt" - "os" - "path/filepath" "github.com/jackc/pgx/v5/pgxpool" "go.mongodb.org/mongo-driver/v2/mongo" @@ -31,16 +29,9 @@ const LegacySQLitePath = "data/gomodel.db" // DefaultSQLitePath returns the database path used when none is configured: // LegacySQLitePath when a ./data directory already exists, otherwise the -// OS-conventional per-user data directory (see platformdir.DataDir). +// OS-conventional per-user data directory (see platformdir.DataFile). func DefaultSQLitePath() string { - if info, err := os.Stat("data"); err == nil && info.IsDir() { - return LegacySQLitePath - } - dir, err := platformdir.DataDir() - if err != nil { - return LegacySQLitePath - } - return filepath.Join(dir, "gomodel.db") + return platformdir.DataFile("gomodel.db") } // Config holds storage configuration diff --git a/run/reload.go b/run/reload.go index d389560ba..826d00e85 100644 --- a/run/reload.go +++ b/run/reload.go @@ -6,6 +6,7 @@ import ( "io" "io/fs" "log/slog" + "maps" "os" "path/filepath" "strconv" @@ -43,22 +44,42 @@ func newDotenv() *dotenv { // environment — and clears whatever the file previously contributed. A file // that cannot be read or parsed is not: it leaves the environment as it stands, // so a half-typed edit does not strip the running configuration. -func (d *dotenv) apply() { +// +// The returned function undoes exactly the changes this call made. A reload +// reads the file before it knows whether the configuration built from it is +// usable, so rejecting that configuration has to put the environment back the +// way the still-running gateway expects it. +func (d *dotenv) apply() (undo func()) { values, err := godotenv.Read(envFile) if err != nil { if !errors.Is(err, fs.ErrNotExist) { slog.Warn("failed to read env file; keeping the current environment", "file", envFile, "error", err) - return + return func() {} } values = map[string]string{} } + // A nil entry records a variable that was not set before this call. + previous := make(map[string]*string) + record := func(key string) { + if _, seen := previous[key]; seen { + return + } + if value, exported := os.LookupEnv(key); exported { + previous[key] = &value + return + } + previous[key] = nil + } + appliedBefore := maps.Clone(d.applied) + for key, value := range values { if _, owned := d.applied[key]; !owned { if _, exported := os.LookupEnv(key); exported { continue } } + record(key) if err := os.Setenv(key, value); err != nil { slog.Warn("failed to apply env file variable", "file", envFile, "variable", key, "error", err) continue @@ -70,12 +91,24 @@ func (d *dotenv) apply() { if _, present := values[key]; present { continue } + record(key) if err := os.Unsetenv(key); err != nil { slog.Warn("failed to unset removed env file variable", "file", envFile, "variable", key, "error", err) continue } delete(d.applied, key) } + + return func() { + for key, value := range previous { + if value == nil { + _ = os.Unsetenv(key) + continue + } + _ = os.Setenv(key, *value) + } + d.applied = appliedBefore + } } // writePIDFile records the running process id so `gomodel --reload` can find @@ -106,6 +139,9 @@ func writePIDFile(path string) (func(), error) { }, nil } +// readPIDFile returns the process id recorded at path. A missing file and a +// file that holds anything else both report why, since both are what an +// operator sees when --reload cannot find the gateway. func readPIDFile(path string) (int, error) { data, err := os.ReadFile(path) if err != nil { diff --git a/run/reload_test.go b/run/reload_test.go index 16423fe27..ad84b46b9 100644 --- a/run/reload_test.go +++ b/run/reload_test.go @@ -5,7 +5,9 @@ import ( "errors" "net" "os" + "os/signal" "path/filepath" + "strings" "sync" "sync/atomic" "testing" @@ -282,3 +284,128 @@ func TestPIDFileRemovalLeavesAnotherInstanceAlone(t *testing.T) { t.Errorf("pid = %d, want 424242", pid) } } + +// A reload reads the environment file before it can know whether the +// configuration built from it works. When it does not, the generation that +// keeps serving must keep the environment it was built with — the operator was +// told the new configuration was rejected. +func TestDotenvApplyUndoRestoresTheEnvironment(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv("GOMODEL_TEST_EXPORTED", "from-environment") + writeEnvFile(t, "GOMODEL_TEST_KEPT=before\nGOMODEL_TEST_DROPPED=present\n") + t.Cleanup(func() { + os.Unsetenv("GOMODEL_TEST_KEPT") + os.Unsetenv("GOMODEL_TEST_DROPPED") + os.Unsetenv("GOMODEL_TEST_ADDED") + }) + + env := newDotenv() + env.apply() + + // The edit a failed reload would have read. + writeEnvFile(t, "GOMODEL_TEST_KEPT=after\nGOMODEL_TEST_ADDED=new\nGOMODEL_TEST_EXPORTED=from-file\n") + undo := env.apply() + if got := os.Getenv("GOMODEL_TEST_KEPT"); got != "after" { + t.Fatalf("edited variable before undo = %q, want %q", got, "after") + } + + undo() + + if got := os.Getenv("GOMODEL_TEST_KEPT"); got != "before" { + t.Errorf("edited variable after undo = %q, want %q", got, "before") + } + if got := os.Getenv("GOMODEL_TEST_DROPPED"); got != "present" { + t.Errorf("removed variable after undo = %q, want %q", got, "present") + } + if _, present := os.LookupEnv("GOMODEL_TEST_ADDED"); present { + t.Error("variable added by the rejected file is still set") + } + if got := os.Getenv("GOMODEL_TEST_EXPORTED"); got != "from-environment" { + t.Errorf("exported variable = %q, want it untouched throughout", got) + } + + // The bookkeeping has to be restored too, or the next reload treats the + // rolled-back variables as none of its business. + writeEnvFile(t, "GOMODEL_TEST_KEPT=third\n") + env.apply() + if got := os.Getenv("GOMODEL_TEST_KEPT"); got != "third" { + t.Errorf("variable after a later reload = %q, want %q", got, "third") + } + if _, present := os.LookupEnv("GOMODEL_TEST_DROPPED"); present { + t.Error("variable dropped from the env file survived the later reload") + } +} + +func TestSendReloadSignal(t *testing.T) { + tests := []struct { + name string + pidFile func(t *testing.T, dir string) string + wantError bool + }{ + { + name: "signals the process named by the pid file", + pidFile: func(t *testing.T, dir string) string { + path := filepath.Join(dir, "gomodel.pid") + remove, err := writePIDFile(path) + if err != nil { + t.Fatalf("writePIDFile() error = %v", err) + } + t.Cleanup(remove) + return path + }, + }, + { + name: "reports a missing pid file", + pidFile: func(t *testing.T, dir string) string { + return filepath.Join(dir, "absent.pid") + }, + wantError: true, + }, + { + name: "reports a pid file that names no process", + pidFile: func(t *testing.T, dir string) string { + path := filepath.Join(dir, "garbage.pid") + if err := os.WriteFile(path, []byte("not-a-pid"), 0o644); err != nil { + t.Fatal(err) + } + return path + }, + wantError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) // no config.yaml here, so only PID_FILE decides the path + t.Setenv("PID_FILE", tt.pidFile(t, dir)) + + // Registered before signalling, exactly as the gateway does it, so a + // delivered SIGHUP is caught here instead of killing the test binary. + delivered := make(chan os.Signal, 1) + signal.Notify(delivered, reloadSignal) + defer signal.Stop(delivered) + + var out strings.Builder + err := sendReloadSignal(&out) + if tt.wantError { + if err == nil { + t.Fatal("sendReloadSignal() error = nil, want an error") + } + return + } + if err != nil { + t.Fatalf("sendReloadSignal() error = %v", err) + } + + select { + case <-delivered: + case <-time.After(5 * time.Second): + t.Fatal("the reload signal was never delivered") + } + if !strings.Contains(out.String(), "reload requested") { + t.Errorf("output = %q, want it to confirm the reload", out.String()) + } + }) + } +} diff --git a/run/run.go b/run/run.go index 65416c447..69da7ca3f 100644 --- a/run/run.go +++ b/run/run.go @@ -127,7 +127,7 @@ func Run(ctx context.Context, opts Options) error { } env := newDotenv() - env.apply() + env.apply() // startup has nothing to roll back to if cliOpts.Health { if err := runHealthProbe(cliOpts.HealthTimeout); err != nil { @@ -220,6 +220,14 @@ func Run(ctx context.Context, opts Options) error { } defer func() { _ = socket.Close() }() + // Claim the reload signal before the pid file exists. The pid file is what + // tells an operator or a process manager that this instance can be signalled, + // and until Notify runs, SIGHUP still carries its default disposition: it + // would kill the gateway instead of reloading it. + reload := make(chan os.Signal, 1) + signal.Notify(reload, reloadSignal) + defer signal.Stop(reload) + removePIDFile, err := writePIDFile(appCfg.Server.PIDFile) if err != nil { slog.Warn("could not write the pid file; --reload will not find this instance", "error", err) @@ -229,19 +237,28 @@ func Run(ctx context.Context, opts Options) error { signalCtx, stop := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM) defer stop() - reload := make(chan os.Signal, 1) - signal.Notify(reload, reloadSignal) - defer signal.Stop(reload) - + // rebuild prepares the next generation. Reading the environment file and + // installing the new logging configuration have to happen first — config.Load + // reads the process environment, and the new log level applies to the build + // itself — but both are process-wide, so a build that fails would otherwise + // leave the generation that keeps serving running under a configuration the + // operator was told had been rejected. Everything is put back instead. rebuild := func() (lifecycleApp, error) { - // The environment file is re-read first so config.Load sees the new - // values; variables exported into the process keep winning over it. - env.apply() + // Variables exported into the process keep winning over the file. + undoEnv := env.apply() + previousLogger := slog.Default() + rollback := func() { + slog.SetDefault(previousLogger) + undoEnv() + } + if err := configureLogging(opts.Stderr); err != nil { + rollback() return nil, err } next, nextCfg, err := build() if err != nil { + rollback() return nil, err } warnAboutStartupOnlySettings(appCfg.Server, nextCfg.Server) @@ -369,6 +386,8 @@ func serveGeneration(ctx context.Context, application lifecycleApp, listener net return startErr } +// shutdownApplicationWithTimeout tears an application down on the one budget +// every teardown gets, whether or not it ever served. func shutdownApplicationWithTimeout(application lifecycleApp) error { shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) defer cancel() diff --git a/run/socket.go b/run/socket.go index 1fb1ff354..506ba61a8 100644 --- a/run/socket.go +++ b/run/socket.go @@ -28,6 +28,7 @@ type boundSocket struct { listener net.Listener } +// listenOn binds address and keeps hold of it for the process's lifetime. func listenOn(address string) (*boundSocket, error) { listener, err := net.Listen("tcp", address) if err != nil { @@ -66,6 +67,8 @@ func (s *boundSocket) next() (net.Listener, error) { return net.Listen("tcp", s.address) } +// Close releases the socket for good, which is the process exiting rather than +// a generation ending. func (s *boundSocket) Close() error { if s.listener != nil { defer func() { s.listener = nil }()