feat(server): reload configuration without a restart via --reload - #644
Conversation
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
📝 WalkthroughWalkthroughThe gateway adds configurable PID files and a ChangesGateway reload
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as gomodel --reload
participant Reload as run reload helpers
participant Gateway as running gateway
participant Socket as boundSocket
participant Generation as replacement generation
CLI->>Reload: read configured PID file
Reload->>Gateway: send SIGHUP
Gateway->>Gateway: reapply dotenv and rebuild configuration
Gateway->>Socket: obtain retained listener
Gateway->>Generation: start with retained listener
Generation-->>Gateway: start result
Gateway->>Gateway: shut down old generation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.template:
- Around line 8-13: Update the PID_FILE documentation comment to state that
changing its value in the environment requires restarting the running gateway to
take effect, while preserving the existing default and empty-value behavior
descriptions.
In `@config/config.example.yaml`:
- Line 18: Update the inline comment for pid_file to state that changing this
setting requires restarting the gateway, while preserving its existing default,
PID_FILE override, and empty-value behavior descriptions.
In `@config/server_test.go`:
- Around line 55-66: Update TestPIDFileEnvOverride to use table-driven subtests
covering both the existing non-empty PID_FILE override and an empty PID_FILE
value. For the empty case, assert that result.Config.Server.PIDFile is empty,
preserving the documented no-pid-file behavior; continue loading configuration
independently for each case.
In `@config/server.go`:
- Around line 50-55: Update the PIDFile documentation in the struct comment and
the corresponding `.env.template`, `config/config.example.yaml`, and
`docs/advanced/configuration.mdx` entries to state that changing
PID_FILE/PIDFile requires a process restart and is not applied by `gomodel
--reload`; preserve the existing behavior and guidance about disabling the PID
file with an empty value.
- Around line 61-74: Extract the shared data-directory resolution used by
DefaultSQLitePath and DefaultPIDFilePath into a helper in internal/platformdir,
preserving the rule that an existing ./data directory or a DataDir error selects
the legacy path. Update both functions to call this helper and append their
respective filenames, keeping the existing path values and behavior unchanged.
In `@docs/advanced/cli.mdx`:
- Around line 122-125: Update the reload documentation around the
GracefulDrainTimeout references to use the operator-facing BASE_PATH-aware
environment variable or YAML configuration key, and link to its definition; if
no such configurable setting exists, remove the setting name while preserving
the explanation of the drain window.
In `@docs/advanced/config-yaml.mdx`:
- Around line 113-117: Update the configuration reload paragraph near the CLI
Operations reference to explicitly note that changes to server.port and
server.pid_file require a full restart and are not applied by gomodel --reload,
while preserving the existing reload behavior for other config.yaml edits.
In `@docs/advanced/configuration.mdx`:
- Line 50: Update the PID_FILE row in the configuration documentation to state
that changing this setting requires restarting the running gateway, while
preserving the existing default-path description.
In `@run/reload_test.go`:
- Around line 81-118: The reload test coverage is missing for sendReloadSignal
and its user-visible success and failure paths. Add table-driven tests that
register signal.Notify with reloadSignal, write the current process PID and
verify sendReloadSignal delivers the signal, and verify a missing PID file
returns an error; clean up signal notifications and temporary PID files within
the tests.
In `@run/run.go`:
- Around line 228-233: Register the reload channel with signal.Notify before the
writePIDFile call in the surrounding startup flow, ensuring reloadSignal is
handled as soon as the pid file becomes visible. Keep the existing signal.Stop
cleanup and other signal context setup intact.
- Around line 235-249: Update the rebuild closure to stage environment changes
in a candidate snapshot instead of calling env.apply() against the live process
before build(). Configure and build using that candidate, then commit the
environment snapshot and logging configuration only after build() succeeds;
preserve the existing appCfg replacement and warning flow, leaving the old
generation and process environment unchanged on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5cbc63e3-5972-401d-a3d6-2aadc0968609
📒 Files selected for processing (15)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/config.goconfig/server.goconfig/server_test.godocs/advanced/cli.mdxdocs/advanced/config-yaml.mdxdocs/advanced/configuration.mdxrun/flags.gorun/lifecycle_test.gorun/reload.gorun/reload_test.gorun/run.gorun/socket.go
| // 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") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare DefaultPIDFilePath with storage.DefaultSQLitePath for duplicated logic.
fd -t f storage.go | xargs -I{} rg -n -A 12 'func DefaultSQLitePath' {}Repository: ENTERPILOT/GoModel
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -t f '(^storage\.go$|server\.go$)'
echo
echo "== config/server.go relevant section =="
cat -n config/server.go | sed -n '1,100p'
echo
echo "== storage storage.go relevant section =="
cat -n storage.go | sed -n '1,70p'
echo
echo "== platformdir DataDir and relevant symbols =="
fd -t f 'platformdir' -d .
for f in $(fd -t f 'platformdir' -d .); do
echo "--- $f"
cat -n "$f" | sed -n '1,120p'
doneRepository: ENTERPILOT/GoModel
Length of output: 5536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config/storage.go =="
cat -n config/storage.go | sed -n '1,90p'
echo
echo "== internal/storage/storage.go relevant section =="
cat -n internal/storage/storage.go | sed -n '1,80p'
echo
echo "== platformdir files =="
fd -t f 'platformdir' . -x sh -c 'echo "--- $1"; cat -n "$1" | sed -n "1,140p"' sh {}Repository: ENTERPILOT/GoModel
Length of output: 10263
Extract the shared data-directory fallback.
DefaultSQLitePath() and DefaultPIDFilePath() use the same resolution rule: return the legacy path when ./data exists, otherwise call platformdir.DataDir() and fall back to the legacy path on error. Move this resolution to one helper, such as in internal/platformdir, so future changes keep both paths in sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/server.go` around lines 61 - 74, Extract the shared data-directory
resolution used by DefaultSQLitePath and DefaultPIDFilePath into a helper in
internal/platformdir, preserving the rule that an existing ./data directory or a
DataDir error selects the legacy path. Update both functions to call this helper
and append their respective filenames, keeping the existing path values and
behavior unchanged.
| 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 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find configuration read lazily from the environment on request paths.
set -euo pipefail
rg -nP --type=go -C3 '\bos\.(Getenv|LookupEnv)\b' -g '!**/*_test.go' -g '!config/**' -g '!run/**'
# httpclient is the known lazy reader referenced by app.New.
fd -t f -p 'httpclient' --exec-batch ast-grep outline --items allRepository: ENTERPILOT/GoModel
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files matching run.go / reload_test.go / cli.mdx =="
git ls-files | rg '(^|/)run/run\.go$|(^|/)run/reload_test\.go$|docs/advanced/cli\.mdx$|(^|/)httpclient'
echo
echo "== run/run.go context =="
sed -n '200,270p' run/run.go
echo
echo "== env apply implementation candidates =="
rg -n --type=go -C4 'func.*apply|type .*Env|LookupEnv|Setenv|Unset|Delete' -g '!**/*_test.go' .
echo
echo "== reload tests around env apply assertions =="
sed -n '1,120p' run/reload_test.goRepository: ENTERPILOT/GoModel
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== load function / env type definitions =="
rg -n --type=go -C3 'type dotenv|func \(.*Load\(|func Load\(|envFile|applied' run config -g '!**/*_test.go'
echo
echo "== run/reload.go =="
sed -n '1,130p' run/reload.go
echo
echo "== docs/advanced/cli.mdx reload section =="
sed -n '90,130p' docs/advanced/cli.mdx
echo
echo "== reload test relevant assertions =="
sed -n '1,130p' run/reload_test.go
echo
echo "== serveUntilShutdown / rebuild call flow =="
rg -n --type=go -C8 'func serveUntilShutdown|type serveArgs|serveUntilShutdown\(|rebuild\(\)|build\(\)' run -g '!**/*_test.go'
echo
echo "== direct os.Getenv/LookupEnv uses in non-test Go files, compact =="
rg -n '\bos\.(Getenv|LookupEnviron|Environ|Setenv|Unsetenv)\b' --type=go -g '!**/*_test.go' -g '!cmd/gomodel/docs/docs.go' internal config run internal/httpclient | head -n 200Repository: ENTERPILOT/GoModel
Length of output: 25698
Do not mutate the live reload environment before the replacement builds.
env.apply() updates and unsets environment variables before build() runs. If build() fails, rebuild() logs the error and the old generation keeps serving, but the process environment already reflects the rejected .env changes. Apply these changes from a candidate environment snapshot and commit the snapshot and logging configuration only after build() succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run/run.go` around lines 235 - 249, Update the rebuild closure to stage
environment changes in a candidate snapshot instead of calling env.apply()
against the live process before build(). Configure and build using that
candidate, then commit the environment snapshot and logging configuration only
after build() succeeds; preserve the existing appCfg replacement and warning
flow, leaving the old generation and process environment unchanged on failure.
Confidence Score: 4/5
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (2)
Reviews (1): Last reviewed commit: "feat(server): reload configuration witho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }() | ||
|
|
||
| startErr := application.Start(context.Background(), addr) | ||
| startErr := application.StartWithListener(context.Background(), listener) |
There was a problem hiding this comment.
Every generation is started through StartWithListener here, but that path creates a bare Echo start configuration rather than the gateway start configuration. The configured header-read and write deadlines, graceful-drain timeout, and shutdown-error handling therefore do not apply to normal or reloaded gateway generations. A real /health request on this path received Echo's 30-second read deadline instead of the configured 10-second header-read deadline.
Context Used: CLAUDE.md (source)
Artifacts
Before fix: listener-start path misses the configured header deadline
- The executed focused Go test started the pre-bound listener path and made a real health request; it failed because the observed read deadline was 30 seconds rather than the configured 10-second header deadline, confirming the omission.
After fix: listener-start path applies configured HTTP deadlines
- The same executed focused Go test passed after the listener path reused the gateway start configuration, showing that configured header-read and write timeout propagation now works.
Focused StartWithListener timeout propagation repro source
- This authored Go test wraps accepted TCP connections, runs the real StartWithListener server path, requests health, and asserts the configured deadlines; it is the executable source for the before and after captures.
Server and run package regression test output after the fix
- The executed `go test ./internal/server ./run` command completed successfully after the focused fix, showing the relevant package regression suite remains green.
| env.apply() | ||
| if err := configureLogging(opts.Stderr); err != nil { | ||
| return nil, err | ||
| } | ||
| next, nextCfg, err := build() |
There was a problem hiding this comment.
rebuild reapplies dotenv values and replaces the process-default logger before building the replacement application. When construction fails, the old generation remains active but neither global is restored. The reproduced live request changed from mark=old to mark=new and began emitting DEBUG output after an invalid reload, although no replacement generation was installed.
Context Used: CLAUDE.md (source)
Artifacts
Executable Go reproduction source for failed reload isolation
- Focused Go test starts the gateway, makes a live request, signals an invalid reload, and observes environment and logger behavior; it is the executable proof harness.
Baseline request before reload
- Executed baseline Go test shows the original generation returned `mark=old` while INFO logging suppressed the request DEBUG record; this establishes the pre-reload behavior.
Failed reload changes retained generation behavior
- Executed failed-reload Go test shows the still-serving generation returned `mark=new` and emitted request DEBUG after invalid replacement configuration; this confirms leaked process-global state.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
run/reload.go (1)
97-103: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake PID-file ownership atomic.
The PID comparison and
os.Removeuse separate pathname operations. Another instance can replace the file afterreadPIDFilesucceeds and before removal. This cleanup then deletes that instance's PID file.Use exclusive creation plus a durable ownership mechanism. Handle stale PID files separately. Do not use a read-then-unlink check as ownership protection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@run/reload.go` around lines 97 - 103, Replace the readPIDFile-then-os.Remove ownership check in the cleanup flow with atomic PID-file ownership using exclusive creation and a durable ownership mechanism. Track and validate ownership through the resulting handle or equivalent mechanism before removal, while handling stale PID files separately; do not rely on separate pathname reads and unlink operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/http_start_test.go`:
- Around line 129-161: Expand
TestNewGatewayStartConfigForListener_KeepsTheServerConfiguration into a
table-driven test covering configured ReadTimeout and the StartWithListener(ctx,
nil) error path. Preserve the existing listener, graceful timeout, shutdown
callback, and BeforeServeFunc assertions, and ensure the nil-listener case
verifies the expected error.
---
Outside diff comments:
In `@run/reload.go`:
- Around line 97-103: Replace the readPIDFile-then-os.Remove ownership check in
the cleanup flow with atomic PID-file ownership using exclusive creation and a
durable ownership mechanism. Track and validate ownership through the resulting
handle or equivalent mechanism before removal, while handling stale PID files
separately; do not rely on separate pathname reads and unlink operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e852a17-8cad-4c5a-a0ec-6be7fe2a87fb
📒 Files selected for processing (5)
internal/server/http.gointernal/server/http_start_test.gorun/reload.gorun/run.gorun/socket.go
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/advanced/cli.mdx`:
- Around line 123-128: Update the reload behavior documentation near the
listener lifetime and no-refusal guarantee to state that it depends on
successful listener descriptor duplication. Document that when duplication
fails, the next generation rebinds the address and connections may be refused
during the rebinding gap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f10c774-5a59-4b60-95fd-00af26f4066a
📒 Files selected for processing (7)
config/server_test.godocs/advanced/cli.mdxrun/lifecycle_test.gorun/reload.gorun/reload_test.gorun/run.gorun/socket.go
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty
|
Review findings addressed in Pre-bound listener bypassed the gateway HTTP server configuration (both reviewers, P1) — real, and already fixed in Failed reload leaked the new environment and logger into the retained generation (both reviewers, P1) — real, fixed in Register the reload signal before writing the pid file — real. The pid file is what advertises that this instance can be signalled, and until
Empty No-refusal guarantee depends on descriptor duplication — qualified in Extract the shared data-directory fallback — done: Coverage for Full suite, Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
run/run.go (1)
259-265: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReload
GOMODEL_DEMO_MODEfor each generation.
demoModeFromEnv()runs once beforebuildis defined. After line 248 applies an updated.env,build()still passes the captured initialdemoModetoapp.New. A reload therefore ignores changes toGOMODEL_DEMO_MODE. The demo-warning lifecycle also remains in its initial state.Resolve demo mode while building each candidate generation. Update the warning lifecycle only after that generation succeeds. Add reload tests for both enabled-to-disabled and disabled-to-enabled transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@run/run.go` around lines 259 - 265, Update the generation builder around build and demoModeFromEnv so each candidate generation resolves GOMODEL_DEMO_MODE from the current environment instead of reusing the initial captured value. Apply the demo-warning lifecycle state only after build succeeds, preserving the existing rollback path on failure. Add reload coverage for both enabled-to-disabled and disabled-to-enabled transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 114: Update the PID_FILE reload documentation to qualify the
listening-socket handoff guarantee: state that no connections are refused only
when listener descriptor duplication succeeds, and that the fallback rebind may
briefly refuse connections. Preserve the existing reload behavior and platform
details.
In `@run/reload_test.go`:
- Around line 339-411: Extend TestSendReloadSignal with a stale-PID case that
writes a valid numeric PID for an exited process, then expects sendReloadSignal
to return an error. Assert the error message instructs the operator to remove
the stale PID file, while preserving the existing missing-file and invalid-PID
cases.
---
Outside diff comments:
In `@run/run.go`:
- Around line 259-265: Update the generation builder around build and
demoModeFromEnv so each candidate generation resolves GOMODEL_DEMO_MODE from the
current environment instead of reusing the initial captured value. Apply the
demo-warning lifecycle state only after build succeeds, preserving the existing
rollback path on failure. Add reload coverage for both enabled-to-disabled and
disabled-to-enabled transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4e88a5e-12d2-4b91-acad-b6593bd901bb
📒 Files selected for processing (15)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/server.goconfig/server_test.godocs/advanced/cli.mdxdocs/advanced/config-yaml.mdxdocs/advanced/configuration.mdxinternal/platformdir/platformdir.gointernal/server/http_start_test.gointernal/storage/storage.gorun/reload.gorun/reload_test.gorun/run.gorun/socket.go
| - `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. `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). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the socket-handoff guarantee.
run/socket.go rebinds the address when listener descriptor duplication fails. During that fallback, connections can be refused. State that the no-refusal guarantee applies only when descriptor duplication succeeds.
As per coding guidelines, “Document new configuration or API behavior and mention relevant provider-specific behavior.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 114, Update the PID_FILE reload documentation to qualify
the listening-socket handoff guarantee: state that no connections are refused
only when listener descriptor duplication succeeds, and that the fallback rebind
may briefly refuse connections. Preserve the existing reload behavior and
platform details.
Source: Coding guidelines
| 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()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add stale PID-file coverage.
The test does not cover a valid PID for a process that has exited. This path reaches the os.ErrProcessDone or syscall.ESRCH handling in sendReloadSignal. Add a case that writes a stale numeric PID and asserts the error tells the operator to remove the stale PID file.
As per coding guidelines, “Add or update tests for behavior changes; cover … error handling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run/reload_test.go` around lines 339 - 411, Extend TestSendReloadSignal with
a stale-PID case that writes a valid numeric PID for an exited process, then
expects sendReloadSignal to return an error. Assert the error message instructs
the operator to remove the stale PID file, while preserving the existing
missing-file and invalid-PID cases.
Source: Coding guidelines
gomodel --reloadnow asks the running gateway to re-read itsconfiguration, the same operation as
nginx -s reload. It resolves thenew
server.pid_file/PID_FILEsetting, signals that process(SIGHUP, so
kill -HUPworks too), and exits.On the signal the gateway re-reads the
.envfile and then reloads thewhole configuration by rebuilding itself, so every setting reloads
rather than a curated subset. Two properties make it safe to run in
production:
configuration that fails to load or initialize leaves the gateway
serving on the one that already works;
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.
PORTandPID_FILEchanges still need a restart and are warnedabout. The pid file defaults to
data/gomodel.pidnext to an existing./datadirectory and to the OS per-user data directory otherwise —the same resolution the SQLite database uses.
Closes #573
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01F5FhrZ1ZchNHmEnvsTXdty
Summary by CodeRabbit
New Features
gomodel --reloadto apply environment and configuration changes without stopping the gateway.Documentation