Skip to content

fix: reload log level on SIGHUP#945

Open
pollychen-lab wants to merge 2 commits into
xataio:mainfrom
pollychen-lab:fix/issue-943-reload-log-level-sighup
Open

fix: reload log level on SIGHUP#945
pollychen-lab wants to merge 2 commits into
xataio:mainfrom
pollychen-lab:fix/issue-943-reload-log-level-sighup

Conversation

@pollychen-lab

@pollychen-lab pollychen-lab commented Jun 26, 2026

Copy link
Copy Markdown

Summary

Reload the active log level when pgstream receives SIGHUP instead of treating it as a shutdown signal.

Related Issue

Fixes #943

Changes

  • Handles SIGHUP separately from shutdown signals.
  • Reloads config and applies a changed logger level through zerolog's global level filter.
  • Rejects invalid log levels and keeps the previous logger level in place.
  • Avoids writing config reload notices to stdout during SIGHUP reloads.
  • Adds focused tests for successful reload, invalid-level preservation, and quiet reload behavior.

Testing

  • docker run --rm -v "$PWD":/src -w /src golang:1.26.4 gofmt -w cmd/root_cmd.go cmd/run_cmd.go cmd/snapshot_cmd.go cmd/root_cmd_test.go cmd/config/config.go internal/log/zerolog/zerolog.go pkg/log/zerolog/logger.go
  • docker run --rm -v "$PWD":/src -w /src golang:1.26.4 go test ./cmd/... ./internal/log/... ./pkg/log/...
  • docker run --rm -v "$PWD":/src -w /src golang:1.26.4 go test -race ./cmd
  • git diff --check

Notes

Local host does not have Go installed, so validation was run in Docker with golang:1.26.4, matching the repository's Go version.

Signed-off-by: Claude Code <claude-code@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbe14e9e84

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cmd/root_cmd.go Outdated
}

func reloadLogLevel(logger *rszerolog.Logger) error {
if err := config.Load(); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid writing config reload notices to stdout

When SIGHUP is used with a config file and the stdout target, this call goes through config.LoadFile, which prints using config file... with fmt.Printf to stdout; the stdout target writes NDJSON events to os.Stdout (pkg/wal/processor/stdout/wal_stdout_writer.go), so every reload injects a non-JSON line into the event stream. This is newly reachable at runtime because reload now calls config.Load() on each SIGHUP.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I added a quiet config reload path and switched the SIGHUP handler to use that, so reloading a config file no longer emits the using config file... notice to stdout. I also added a regression test that captures stdout during reload and verifies it stays empty.

Comment thread cmd/root_cmd.go Outdated
return nil
}

*logger = logger.Level(level)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard logger mutation during SIGHUP reloads

When a SIGHUP arrives while run or snapshot is actively logging, the reload goroutine overwrites the shared *zerolog.Logger struct, while stdLogger and all WithFields loggers keep the same pointer and can read from it concurrently. That introduces a data race on the logger fields during normal logging; use a synchronized/atomic level mechanism or replace loggers without mutating the shared struct in place.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I changed the reload path so it no longer overwrites the shared *zerolog.Logger. The logger now keeps a stable trace-level instance and reload updates zerolog’s global level filter instead, which avoids mutating the logger struct while other goroutines may be logging. I also ran go test -race ./cmd successfully.

Signed-off-by: Claude Code <claude-code@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reload log level on SIGHUP

2 participants