Thread-safe config reads and SIGHUP reload for HTTP API server#89
Open
mason-sharp wants to merge 1 commit intomainfrom
Open
Thread-safe config reads and SIGHUP reload for HTTP API server#89mason-sharp wants to merge 1 commit intomainfrom
mason-sharp wants to merge 1 commit intomainfrom
Conversation
Replace all bare config.Cfg reads with config.Get() across auth.go, table_diff.go, merkle.go, listen.go, and setup.go to eliminate data races with config.Set() during SIGHUP reload. Switch HTTP API handlers from stale s.cfg snapshot to config.Get() so reloaded values (block size, concurrency, timeouts, etc.) take effect on the next request. Remove the cfg field from APIServer. Add SIGHUP config reload handling for both API-only entry points: ./ace server (StartAPIServerCLI) and ./ace start --component=api. Invalid configs are rejected with the previous config retained. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR refactors configuration access throughout the codebase from static snapshots to a centralized runtime accessor, enabling hot-reload of configuration via SIGHUP signals. Changes include removing stored config from APIServer struct, updating all config access patterns to use Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/consistency/diff/table_diff.go (1)
730-735: Add a nil-config guard in validation path.
cfg.TableDiffis dereferenced immediately; returning a clear validation error is safer than panicking.Proposed fix
cfg := config.Get() + if cfg == nil { + return fmt.Errorf("configuration not loaded") + } if t.BlockSize > cfg.TableDiff.MaxBlockSize && !t.OverrideBlockSize { return fmt.Errorf("block row size should be <= %d", cfg.TableDiff.MaxBlockSize) }Also applies to: 741-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/diff/table_diff.go` around lines 730 - 735, The validation currently dereferences cfg.TableDiff without checking for nil; update the validation path (the block where cfg := config.Get() compares t.BlockSize against cfg.TableDiff.MaxBlockSize/MinBlockSize and checks t.OverrideBlockSize) to first guard that cfg.TableDiff != nil and return a clear error (e.g., "table diff configuration missing") if it is nil; apply the same nil-config guard to the other identical check later (the code around the second occurrence referencing cfg.TableDiff at lines ~741-743) so neither MaxBlockSize nor MinBlockSize dereferences can panic.internal/infra/cdc/setup.go (1)
42-43: Guardconfig.Get()before dereferencing.
config.Get().MTree.CDCcan panic when config is unavailable. Returning an explicit error is safer.Proposed fix
- cfg := config.Get().MTree.CDC + rootCfg := config.Get() + if rootCfg == nil { + return 0, fmt.Errorf("configuration not loaded") + } + cfg := rootCfg.MTree.CDC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infra/cdc/setup.go` around lines 42 - 43, Guard the result of config.Get() and its nested fields before dereferencing: replace the direct access cfg := config.Get().MTree.CDC and slot := cfg.SlotName with a nil-check flow that obtains root := config.Get(), returns an explicit error if root is nil or if root.MTree or root.MTree.CDC is nil, then assigns cfg := root.MTree.CDC and slot := cfg.SlotName; ensure the containing function returns an error when config is unavailable rather than letting a panic occur.docs/scheduling.md (1)
44-47: Add a concrete command example for sending SIGHUP.Consider including one explicit example (e.g.,
kill -HUP <pid>) so operators can apply this immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scheduling.md` around lines 44 - 47, Update the "Signaling the Scheduler to Reload the Configuration" section to include a concrete command example such as "kill -HUP <pid>" and an alternative like "pkill -HUP <process_name>", plus a brief note how to locate the PID (e.g., pgrep or systemctl show) and mention the systemd variant "systemctl kill -s HUP <service>" so operators immediately have actionable commands; place this immediately after the sentence that explains using SIGHUP.internal/api/http/handler.go (1)
223-223: Use one config snapshot per request to avoid mixed-version task settings.At Line 223, Line 363, Line 444, Line 509, Line 584, Line 666, Line 744, Line 802, Line 866, Line 937, Line 1012, and Line 1091, each handler reads
config.Get()independently. Sincepkg/config/config.go:116-179allowsSet()to swap config between reads, a single request can assemble a task with mixed old/new config values.♻️ Suggested pattern (snapshot once, reuse)
func (s *APIServer) handleTableDiff(w http.ResponseWriter, r *http.Request) { + cfg := config.Get() + if cfg == nil { + writeError(w, http.StatusServiceUnavailable, "configuration unavailable") + return + } + ... - task.BlockSize = s.resolveBlockSize(req.BlockSize) - task.ConcurrencyFactor = s.resolveConcurrency(req.Concurrency) - task.CompareUnitSize = s.resolveCompareUnitSize(req.CompareUnitSize) - task.MaxDiffRows = s.resolveMaxDiffRows(req.MaxDiffRows) + task.BlockSize = s.resolveBlockSize(cfg, req.BlockSize) + task.ConcurrencyFactor = s.resolveConcurrency(cfg, req.Concurrency) + task.CompareUnitSize = s.resolveCompareUnitSize(cfg, req.CompareUnitSize) + task.MaxDiffRows = s.resolveMaxDiffRows(cfg, req.MaxDiffRows) ... - task.TaskStorePath = config.Get().Server.TaskStorePath + task.TaskStorePath = cfg.Server.TaskStorePath } -func (s *APIServer) resolveBlockSize(requested int) int { +func (s *APIServer) resolveBlockSize(cfg *config.Config, requested int) int { if requested > 0 { return requested } - if cfg := config.Get(); cfg != nil && cfg.TableDiff.DiffBlockSize > 0 { + if cfg != nil && cfg.TableDiff.DiffBlockSize > 0 { return cfg.TableDiff.DiffBlockSize } return 100000 }Also applies to: 363-363, 444-444, 509-509, 584-584, 666-666, 744-744, 802-802, 866-866, 937-937, 1012-1012, 1091-1091
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/http/handler.go` at line 223, Multiple places in handler.go call config.Get() per-use (e.g., setting task.TaskStorePath = config.Get().Server.TaskStorePath) which can mix config versions during a request; fix by taking a single snapshot of the config at the start of each request handler (e.g., cfg := config.Get()) and then replace subsequent config.Get() calls in that handler with cfg (use cfg.Server.TaskStorePath, cfg.OtherField, etc.), ensuring all task assembly and reads (including where task.TaskStorePath is set) consistently use the same cfg for the duration of the request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/http/config_reload_test.go`:
- Around line 16-20: The test cleanup currently calls
config.Set(&config.Config{}) when original == nil, which mutates global state;
instead, restore the exact prior state by calling config.Set(original) in all
cases (i.e., allow original to be nil) so that when original is nil the global
remains nil; update the cleanup logic around the original variable and the
config.Set call (references: original, config.Set, config.Config) to ensure no
cross-test pollution.
In `@internal/cli/cli.go`:
- Around line 1456-1462: The reload path currently calls config.Reload and
immediately config.Set the newCfg without validating it; update the reload logic
(the block that calls config.Reload/newCfg and the similar block around lines
where this appears) to validate the reloaded config with
canStartAPIServer(newCfg) (the same check used at startup) before calling
config.Set, and if validation fails log an error with details and continue
without replacing the live config; ensure you reference the functions
config.Reload, config.Set and canStartAPIServer in the change and preserve the
existing success/failure logging behavior.
In `@internal/consistency/mtree/merkle.go`:
- Around line 50-52: The aceSchema() helper currently reads
config.Get().MTree.Schema on every call which can drift during reloads; instead
snapshot the schema once at task/run start and reuse that value. Modify call
sites to accept a schema parameter (or store the snapshot on a per-task struct)
and remove direct use of config.Get().MTree.Schema from aceSchema()/caller code
so aceSchema() returns the pre-captured string (or remove aceSchema and use the
injected snapshot); reference aceSchema(), config.Get().MTree.Schema and any
task/run struct or function that orchestrates the operation to propagate and use
the single captured schema value.
In `@internal/infra/cdc/listen.go`:
- Around line 61-63: The bug is that processChanges reads live config via
config.Get() allowing mid-stream schema changes; instead, capture the schema
once in processReplicationStream and pass it down: modify
processReplicationStream to read cfg := config.Get().MTree.CDC (or specifically
mtreeSchema := cfg.SomeSchemaField) before starting the stream and change the
signature of processChanges(ctx context.Context, pool *pgxpool.Pool, changes
[]cdcMsg, mtreeSchema string) to accept the schema, then update all calls to
processChanges in this file (and related call sites e.g., where
processReplicationStream invokes it) so processChanges uses the passed
mtreeSchema instead of calling config.Get() internally. Ensure any other uses of
config.Get() in this CDC path are replaced with the captured value to prevent
mixed-schema writes.
---
Nitpick comments:
In `@docs/scheduling.md`:
- Around line 44-47: Update the "Signaling the Scheduler to Reload the
Configuration" section to include a concrete command example such as "kill -HUP
<pid>" and an alternative like "pkill -HUP <process_name>", plus a brief note
how to locate the PID (e.g., pgrep or systemctl show) and mention the systemd
variant "systemctl kill -s HUP <service>" so operators immediately have
actionable commands; place this immediately after the sentence that explains
using SIGHUP.
In `@internal/api/http/handler.go`:
- Line 223: Multiple places in handler.go call config.Get() per-use (e.g.,
setting task.TaskStorePath = config.Get().Server.TaskStorePath) which can mix
config versions during a request; fix by taking a single snapshot of the config
at the start of each request handler (e.g., cfg := config.Get()) and then
replace subsequent config.Get() calls in that handler with cfg (use
cfg.Server.TaskStorePath, cfg.OtherField, etc.), ensuring all task assembly and
reads (including where task.TaskStorePath is set) consistently use the same cfg
for the duration of the request.
In `@internal/consistency/diff/table_diff.go`:
- Around line 730-735: The validation currently dereferences cfg.TableDiff
without checking for nil; update the validation path (the block where cfg :=
config.Get() compares t.BlockSize against
cfg.TableDiff.MaxBlockSize/MinBlockSize and checks t.OverrideBlockSize) to first
guard that cfg.TableDiff != nil and return a clear error (e.g., "table diff
configuration missing") if it is nil; apply the same nil-config guard to the
other identical check later (the code around the second occurrence referencing
cfg.TableDiff at lines ~741-743) so neither MaxBlockSize nor MinBlockSize
dereferences can panic.
In `@internal/infra/cdc/setup.go`:
- Around line 42-43: Guard the result of config.Get() and its nested fields
before dereferencing: replace the direct access cfg := config.Get().MTree.CDC
and slot := cfg.SlotName with a nil-check flow that obtains root :=
config.Get(), returns an explicit error if root is nil or if root.MTree or
root.MTree.CDC is nil, then assigns cfg := root.MTree.CDC and slot :=
cfg.SlotName; ensure the containing function returns an error when config is
unavailable rather than letting a panic occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2543e0c-d27d-462b-8870-123b5d8e6bb4
📒 Files selected for processing (10)
docs/scheduling.mdinternal/api/http/config_reload_test.gointernal/api/http/handler.gointernal/api/http/server.gointernal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.gointernal/infra/cdc/listen.gointernal/infra/cdc/setup.gointernal/infra/db/auth.go
💤 Files with no reviewable changes (1)
- internal/api/http/server.go
Comment on lines
+16
to
+20
| if original != nil { | ||
| config.Set(original) | ||
| } else { | ||
| config.Set(&config.Config{}) | ||
| } |
There was a problem hiding this comment.
Restore exact prior global state in test cleanup.
When original == nil, setting &config.Config{} changes post-test global semantics. Restore nil instead to avoid cross-test pollution.
Proposed fix
t.Cleanup(func() {
if original != nil {
config.Set(original)
} else {
- config.Set(&config.Config{})
+ config.Set(nil)
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if original != nil { | |
| config.Set(original) | |
| } else { | |
| config.Set(&config.Config{}) | |
| } | |
| if original != nil { | |
| config.Set(original) | |
| } else { | |
| config.Set(nil) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/http/config_reload_test.go` around lines 16 - 20, The test
cleanup currently calls config.Set(&config.Config{}) when original == nil, which
mutates global state; instead, restore the exact prior state by calling
config.Set(original) in all cases (i.e., allow original to be nil) so that when
original is nil the global remains nil; update the cleanup logic around the
original variable and the config.Set call (references: original, config.Set,
config.Config) to ensure no cross-test pollution.
Comment on lines
+1456
to
+1462
| newCfg, err := config.Reload(config.CfgPath) | ||
| if err != nil { | ||
| logger.Error("api: config reload failed (keeping current config): %v", err) | ||
| continue | ||
| } | ||
| config.Set(newCfg) | ||
| logger.Info("api: configuration reloaded successfully") |
There was a problem hiding this comment.
Validate reloaded API config before config.Set() (parity with startup checks).
Both API reload loops set the new config without canStartAPIServer gating. This can accept an API-invalid config at runtime instead of retaining the previous one.
Proposed fix
newCfg, err := config.Reload(config.CfgPath)
if err != nil {
logger.Error("api: config reload failed (keeping current config): %v", err)
continue
}
+ if ok, vErr := canStartAPIServer(newCfg); !ok {
+ logger.Error("api: new config rejected (keeping current config): %v", vErr)
+ continue
+ }
config.Set(newCfg)
logger.Info("api: configuration reloaded successfully")Also applies to: 1602-1608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/cli.go` around lines 1456 - 1462, The reload path currently
calls config.Reload and immediately config.Set the newCfg without validating it;
update the reload logic (the block that calls config.Reload/newCfg and the
similar block around lines where this appears) to validate the reloaded config
with canStartAPIServer(newCfg) (the same check used at startup) before calling
config.Set, and if validation fails log an error with details and continue
without replacing the live config; ensure you reference the functions
config.Reload, config.Set and canStartAPIServer in the change and preserve the
existing success/failure logging behavior.
Comment on lines
50
to
52
| func aceSchema() string { | ||
| return config.Cfg.MTree.Schema | ||
| return config.Get().MTree.Schema | ||
| } |
There was a problem hiding this comment.
Avoid live schema lookup on every aceSchema() call.
This introduces per-call config drift risk during reloads: one long operation can target different schemas. Snapshot the schema once per task/run and reuse it consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/consistency/mtree/merkle.go` around lines 50 - 52, The aceSchema()
helper currently reads config.Get().MTree.Schema on every call which can drift
during reloads; instead snapshot the schema once at task/run start and reuse
that value. Modify call sites to accept a schema parameter (or store the
snapshot on a per-task struct) and remove direct use of
config.Get().MTree.Schema from aceSchema()/caller code so aceSchema() returns
the pre-captured string (or remove aceSchema and use the injected snapshot);
reference aceSchema(), config.Get().MTree.Schema and any task/run struct or
function that orchestrates the operation to propagate and use the single
captured schema value.
Comment on lines
+61
to
63
| cfg := config.Get().MTree.CDC | ||
| publication := cfg.PublicationName | ||
| slotName := cfg.SlotName |
There was a problem hiding this comment.
Snapshot config-derived schema once per stream to avoid mixed-schema writes.
Using live config.Get() inside processChanges allows schema changes mid-stream after SIGHUP, so one CDC run can write to different schemas. Capture schema once in processReplicationStream and pass it through.
Proposed direction
- cfg := config.Get().MTree.CDC
+ rootCfg := config.Get()
+ if rootCfg == nil {
+ return fmt.Errorf("configuration not loaded")
+ }
+ cfg := rootCfg.MTree.CDC
+ mtreeSchema := rootCfg.MTree.Schema
...
- if err := processChanges(processingCtx, pool, c); err != nil {
+ if err := processChanges(processingCtx, pool, c, mtreeSchema); err != nil {// apply outside selected range
func processChanges(ctx context.Context, pool *pgxpool.Pool, changes []cdcMsg, mtreeSchema string) error {
// use mtreeSchema directly, no config.Get() here
}Also applies to: 554-556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/infra/cdc/listen.go` around lines 61 - 63, The bug is that
processChanges reads live config via config.Get() allowing mid-stream schema
changes; instead, capture the schema once in processReplicationStream and pass
it down: modify processReplicationStream to read cfg := config.Get().MTree.CDC
(or specifically mtreeSchema := cfg.SomeSchemaField) before starting the stream
and change the signature of processChanges(ctx context.Context, pool
*pgxpool.Pool, changes []cdcMsg, mtreeSchema string) to accept the schema, then
update all calls to processChanges in this file (and related call sites e.g.,
where processReplicationStream invokes it) so processChanges uses the passed
mtreeSchema instead of calling config.Get() internally. Ensure any other uses of
config.Get() in this CDC path are replaced with the captured value to prevent
mixed-schema writes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A follow up to the SIGHUP changes in PR #84
Replace all bare config.Cfg reads with config.Get() across auth.go, table_diff.go, merkle.go, listen.go, and setup.go to eliminate data races with config.Set() during SIGHUP reload.
Switch HTTP API handlers from stale s.cfg snapshot to config.Get() so reloaded values (block size, concurrency, timeouts, etc.) take effect on the next request. Remove the cfg field from APIServer.
Add SIGHUP config reload handling for both API-only entry points: ./ace server (StartAPIServerCLI) and ./ace start --component=api. Invalid configs are rejected with the previous config retained.
While the test included is basic, there was testing done to verify the change.