Skip to content

Commit 1a09953

Browse files
authored
refactor: remove redundant config default fallbacks in server and launcher (#3032)
## Summary Removes duplicated config default logic identified in #2983 / #2984. Consumer code in `unified.go` and `launcher.go` re-implemented config field defaults that the config loading layer already guarantees. ## Changes ### New: `Config.EnsureGatewayDefaults()` (`config_core.go`) Exported method that guarantees `cfg.Gateway` is non-nil and all gateway-level + feature defaults are applied. This makes the config loading contract explicit and safe for any consumer — including test code that constructs configs manually without going through `LoadFromFile`/`LoadFromStdin`. ### Simplified: `internal/server/unified.go` **Before** (3 nil-guard + fallback blocks, 18 lines): ```go payloadDir := config.DefaultPayloadDir if cfg.Gateway != nil && cfg.Gateway.PayloadDir != "" { payloadDir = cfg.Gateway.PayloadDir } // ... repeated for PayloadPathPrefix, PayloadSizeThreshold ``` **After** (direct field access, 3 lines): ```go cfg.EnsureGatewayDefaults() payloadDir := cfg.Gateway.PayloadDir payloadPathPrefix := cfg.Gateway.PayloadPathPrefix payloadSizeThreshold := cfg.Gateway.PayloadSizeThreshold ``` ### Simplified: `internal/launcher/launcher.go` **Before** (nil-guard + fallback + branch logging, 8 lines): ```go startupTimeout := time.Duration(config.DefaultStartupTimeout) * time.Second if cfg.Gateway != nil && cfg.Gateway.StartupTimeout > 0 { startupTimeout = time.Duration(cfg.Gateway.StartupTimeout) * time.Second } ``` **After** (direct access, 2 lines): ```go cfg.EnsureGatewayDefaults() startupTimeout := time.Duration(cfg.Gateway.StartupTimeout) * time.Second ``` ### Updated: `launcher_test.go` Updated test name and comment for `TestLauncher_TimeoutWithNilGateway` to document that `EnsureGatewayDefaults` now handles the nil case. ## Why EnsureGatewayDefaults instead of just removing nil-guards? 100+ test files construct `config.Config{}` without setting `Gateway`. Rather than touching all of them, `EnsureGatewayDefaults()` provides a single defensive call that makes the contract explicit while keeping existing tests working. Closes #2983 Closes #2984
2 parents 2e69e37 + 046ef0a commit 1a09953

4 files changed

Lines changed: 30 additions & 27 deletions

File tree

internal/config/config_core.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,19 @@ func applyGatewayDefaults(cfg *GatewayConfig) {
198198
}
199199
}
200200

201+
// EnsureGatewayDefaults guarantees that cfg.Gateway is non-nil and that all
202+
// gateway-level fields have sensible defaults applied. This matches the
203+
// invariants enforced by the standard loaders (LoadFromFile, LoadFromStdin),
204+
// and can be used by callers that construct Config values manually (e.g. in
205+
// tests) to avoid nil-pointer panics and ensure consistent defaults.
206+
func (cfg *Config) EnsureGatewayDefaults() {
207+
if cfg.Gateway == nil {
208+
cfg.Gateway = &GatewayConfig{}
209+
}
210+
applyGatewayDefaults(cfg.Gateway)
211+
applyDefaults(cfg)
212+
}
213+
201214
// LoadFromFile loads configuration from a TOML file.
202215
//
203216
// This function uses the BurntSushi/toml v1.6.0+ parser with TOML 1.1 support,

internal/launcher/launcher.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,13 @@ func New(ctx context.Context, cfg *config.Config) *Launcher {
5555
log.Println("[LAUNCHER] Detected running inside a container")
5656
}
5757

58-
// Get startup timeout from config, default to config.DefaultStartupTimeout seconds
59-
startupTimeout := time.Duration(config.DefaultStartupTimeout) * time.Second
60-
if cfg.Gateway != nil && cfg.Gateway.StartupTimeout > 0 {
61-
startupTimeout = time.Duration(cfg.Gateway.StartupTimeout) * time.Second
62-
logLauncher.Printf("Using configured startup timeout: %v", startupTimeout)
63-
} else {
64-
logLauncher.Printf("Using default startup timeout: %v", startupTimeout)
65-
}
58+
// Guarantee cfg.Gateway is non-nil with defaults applied.
59+
// LoadFromFile/LoadFromStdin already ensure this, but tests may
60+
// construct configs manually without going through the load path.
61+
cfg.EnsureGatewayDefaults()
62+
63+
startupTimeout := time.Duration(cfg.Gateway.StartupTimeout) * time.Second
64+
logLauncher.Printf("Using startup timeout: %v", startupTimeout)
6665

6766
// Initialize OIDC provider from environment if available
6867
var oidcProvider *oidc.Provider

internal/launcher/launcher_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,7 @@ func TestGetOrLaunchForSession_SessionReuse(t *testing.T) {
527527
ctx := context.Background()
528528
cfg := &config.Config{
529529
Servers: map[string]*config.ServerConfig{},
530+
Gateway: &config.GatewayConfig{StartupTimeout: config.DefaultStartupTimeout},
530531
}
531532
l := New(ctx, cfg)
532533
defer l.Close()
@@ -606,7 +607,8 @@ func TestLauncher_StartupTimeout(t *testing.T) {
606607
}
607608

608609
func TestLauncher_TimeoutWithNilGateway(t *testing.T) {
609-
// Test that launcher uses default timeout when Gateway config is nil
610+
// Test that launcher uses default timeout when Gateway config is nil.
611+
// EnsureGatewayDefaults is called by launcher.New, so nil Gateway is safe.
610612
ctx := context.Background()
611613
cfg := &config.Config{
612614
Servers: map[string]*config.ServerConfig{
@@ -615,7 +617,7 @@ func TestLauncher_TimeoutWithNilGateway(t *testing.T) {
615617
URL: "http://example.com",
616618
},
617619
},
618-
Gateway: nil, // No gateway config
620+
Gateway: nil, // No gateway config — EnsureGatewayDefaults fills defaults
619621
}
620622

621623
l := New(ctx, cfg)

internal/server/unified.go

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -110,25 +110,14 @@ type UnifiedServer struct {
110110
// NewUnified creates a new unified MCP server
111111
func NewUnified(ctx context.Context, cfg *config.Config) (*UnifiedServer, error) {
112112
logUnified.Printf("Creating new unified server: sequentialLaunch=%v, servers=%d", cfg.SequentialLaunch, len(cfg.Servers))
113-
l := launcher.New(ctx, cfg)
114-
115-
// Get payload directory from config, with fallback to default
116-
payloadDir := config.DefaultPayloadDir
117-
if cfg.Gateway != nil && cfg.Gateway.PayloadDir != "" {
118-
payloadDir = cfg.Gateway.PayloadDir
119-
}
120113

121-
// Get payload path prefix from config (empty by default)
122-
payloadPathPrefix := ""
123-
if cfg.Gateway != nil && cfg.Gateway.PayloadPathPrefix != "" {
124-
payloadPathPrefix = cfg.Gateway.PayloadPathPrefix
125-
}
114+
l := launcher.New(ctx, cfg)
126115

127-
// Get payload size threshold from config, with fallback to default
128-
payloadSizeThreshold := config.DefaultPayloadSizeThreshold
129-
if cfg.Gateway != nil && cfg.Gateway.PayloadSizeThreshold > 0 {
130-
payloadSizeThreshold = cfg.Gateway.PayloadSizeThreshold
131-
}
116+
// Config loading guarantees cfg.Gateway is non-nil and all fields
117+
// have defaults applied via applyGatewayDefaults/applyDefaults.
118+
payloadDir := cfg.Gateway.PayloadDir
119+
payloadPathPrefix := cfg.Gateway.PayloadPathPrefix
120+
payloadSizeThreshold := cfg.Gateway.PayloadSizeThreshold
132121
logUnified.Printf("Payload configuration: dir=%s, pathPrefix=%s, sizeThreshold=%d bytes (%.2f KB)",
133122
payloadDir, payloadPathPrefix, payloadSizeThreshold, float64(payloadSizeThreshold)/1024)
134123

0 commit comments

Comments
 (0)