Skip to content

Commit 45a06d9

Browse files
Dumbrisclaude
andauthored
fix(046): persist LauncherWaitTimeout to BBolt + regenerate OAS (#454)
* fix(046): persist LauncherWaitTimeout to BBolt + regenerate OAS - Add LauncherWaitTimeout to UpstreamRecord so launcher servers added via the REST upstream_servers API retain the timeout across restarts. Wired through SaveUpstreamServer / GetUpstreamServer / ListUpstreamServers, matching the ReconnectOnUse precedent from spec 354. - Whitelist LauncherWaitTimeout in TestSaveServerSyncFieldCoverage so the ServerConfig field-coverage guard accepts the new field. - Regenerate oas/swagger.yaml + oas/docs.go so the Verify OpenAPI Artifacts CI step stops complaining about drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(046): drain launcher log pumps before cmd.Wait() to stop CI flake reap() called cmd.Wait() before pumpWG.Wait(). Per exec.Cmd docs, Wait closes the parent-side stdout/stderr pipes as part of its cleanup — so if Wait ran before both pump goroutines reached EOF, the pump scanners could race against that close and observe EOF with buffered child output still in the kernel pipe, silently dropping it. CI repro: TestSpawn_LogSinkCaptured on Linux runners (Unit Tests amd64 and Build Binaries linux-arm64) saw the sink contain only the startup banner, no [launcher stdout]/[launcher stderr] lines. Locally the test passed because timing happened to favor pumps. 20x -race iterations green after flipping the order. Pumps naturally hit EOF when the child exits and the kernel propagates the close — calling Wait() is not what unblocks them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(046): switch TestWaitForURL_InfersDefaultPort host to TEST-NET-1 Windows GitHub runners have something bound on 127.0.0.1:80 (IIS/WinNAT default), so the test's "should be unreachable" assertion failed: the dial succeeded and WaitForURL returned nil where the test expected a "not reachable" error. Switched the host to 192.0.2.1 (TEST-NET-1, RFC 5737) — reserved for documentation and guaranteed non-routable, so the dial fails consistently on every OS while still exercising default-port inference from the URL scheme. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Code <noreply@anthropic.com>
1 parent 31e4887 commit 45a06d9

7 files changed

Lines changed: 107 additions & 83 deletions

File tree

internal/storage/async_ops_test.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -228,23 +228,24 @@ func TestSaveServerSyncPreservesNilFields(t *testing.T) {
228228
func TestSaveServerSyncFieldCoverage(t *testing.T) {
229229
// List of ServerConfig fields that ARE expected to be copied
230230
expectedFields := map[string]bool{
231-
"Name": true,
232-
"URL": true,
233-
"Protocol": true,
234-
"Command": true,
235-
"Args": true,
236-
"WorkingDir": true,
237-
"Env": true,
238-
"Headers": true,
239-
"OAuth": true,
240-
"Enabled": true,
241-
"Quarantined": true,
242-
"Created": true,
243-
"Updated": true, // Updated is set by saveServerSync, not copied
244-
"Isolation": true,
245-
"Shared": true, // Teams-only: persisted in JSON config, not in BBolt
246-
"SkipQuarantine": true, // Spec 032: runtime-only field, not persisted to BBolt
247-
"ReconnectOnUse": true, // Spec 354: persisted to BBolt for on-demand reconnection
231+
"Name": true,
232+
"URL": true,
233+
"Protocol": true,
234+
"Command": true,
235+
"Args": true,
236+
"WorkingDir": true,
237+
"Env": true,
238+
"Headers": true,
239+
"OAuth": true,
240+
"Enabled": true,
241+
"Quarantined": true,
242+
"Created": true,
243+
"Updated": true, // Updated is set by saveServerSync, not copied
244+
"Isolation": true,
245+
"Shared": true, // Teams-only: persisted in JSON config, not in BBolt
246+
"SkipQuarantine": true, // Spec 032: runtime-only field, not persisted to BBolt
247+
"ReconnectOnUse": true, // Spec 354: persisted to BBolt for on-demand reconnection
248+
"LauncherWaitTimeout": true, // Spec 046: persisted to BBolt so REST-API-added launcher servers survive restarts
248249
}
249250

250251
// Get all fields from ServerConfig

internal/storage/manager.go

Lines changed: 49 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -85,22 +85,23 @@ func (m *Manager) SaveUpstreamServer(serverConfig *config.ServerConfig) error {
8585
defer m.mu.Unlock()
8686

8787
record := &UpstreamRecord{
88-
ID: serverConfig.Name, // Use name as ID for simplicity
89-
Name: serverConfig.Name,
90-
URL: serverConfig.URL,
91-
Protocol: serverConfig.Protocol,
92-
Command: serverConfig.Command,
93-
Args: serverConfig.Args,
94-
WorkingDir: serverConfig.WorkingDir,
95-
Env: serverConfig.Env,
96-
Headers: serverConfig.Headers,
97-
OAuth: serverConfig.OAuth,
98-
Enabled: serverConfig.Enabled,
99-
Quarantined: serverConfig.Quarantined,
100-
Created: serverConfig.Created,
101-
Updated: time.Now(),
102-
Isolation: serverConfig.Isolation,
103-
ReconnectOnUse: serverConfig.ReconnectOnUse,
88+
ID: serverConfig.Name, // Use name as ID for simplicity
89+
Name: serverConfig.Name,
90+
URL: serverConfig.URL,
91+
Protocol: serverConfig.Protocol,
92+
Command: serverConfig.Command,
93+
Args: serverConfig.Args,
94+
WorkingDir: serverConfig.WorkingDir,
95+
Env: serverConfig.Env,
96+
Headers: serverConfig.Headers,
97+
OAuth: serverConfig.OAuth,
98+
Enabled: serverConfig.Enabled,
99+
Quarantined: serverConfig.Quarantined,
100+
Created: serverConfig.Created,
101+
Updated: time.Now(),
102+
Isolation: serverConfig.Isolation,
103+
ReconnectOnUse: serverConfig.ReconnectOnUse,
104+
LauncherWaitTimeout: serverConfig.LauncherWaitTimeout,
104105
}
105106

106107
return m.db.SaveUpstream(record)
@@ -117,21 +118,22 @@ func (m *Manager) GetUpstreamServer(name string) (*config.ServerConfig, error) {
117118
}
118119

119120
return &config.ServerConfig{
120-
Name: record.Name,
121-
URL: record.URL,
122-
Protocol: record.Protocol,
123-
Command: record.Command,
124-
Args: record.Args,
125-
WorkingDir: record.WorkingDir,
126-
Env: record.Env,
127-
Headers: record.Headers,
128-
OAuth: record.OAuth,
129-
Enabled: record.Enabled,
130-
Quarantined: record.Quarantined,
131-
Created: record.Created,
132-
Updated: record.Updated,
133-
Isolation: record.Isolation,
134-
ReconnectOnUse: record.ReconnectOnUse,
121+
Name: record.Name,
122+
URL: record.URL,
123+
Protocol: record.Protocol,
124+
Command: record.Command,
125+
Args: record.Args,
126+
WorkingDir: record.WorkingDir,
127+
Env: record.Env,
128+
Headers: record.Headers,
129+
OAuth: record.OAuth,
130+
Enabled: record.Enabled,
131+
Quarantined: record.Quarantined,
132+
Created: record.Created,
133+
Updated: record.Updated,
134+
Isolation: record.Isolation,
135+
ReconnectOnUse: record.ReconnectOnUse,
136+
LauncherWaitTimeout: record.LauncherWaitTimeout,
135137
}, nil
136138
}
137139

@@ -148,21 +150,22 @@ func (m *Manager) ListUpstreamServers() ([]*config.ServerConfig, error) {
148150
var servers []*config.ServerConfig
149151
for _, record := range records {
150152
servers = append(servers, &config.ServerConfig{
151-
Name: record.Name,
152-
URL: record.URL,
153-
Protocol: record.Protocol,
154-
Command: record.Command,
155-
Args: record.Args,
156-
WorkingDir: record.WorkingDir,
157-
Env: record.Env,
158-
Headers: record.Headers,
159-
OAuth: record.OAuth,
160-
Enabled: record.Enabled,
161-
Quarantined: record.Quarantined,
162-
Created: record.Created,
163-
Updated: record.Updated,
164-
Isolation: record.Isolation,
165-
ReconnectOnUse: record.ReconnectOnUse,
153+
Name: record.Name,
154+
URL: record.URL,
155+
Protocol: record.Protocol,
156+
Command: record.Command,
157+
Args: record.Args,
158+
WorkingDir: record.WorkingDir,
159+
Env: record.Env,
160+
Headers: record.Headers,
161+
OAuth: record.OAuth,
162+
Enabled: record.Enabled,
163+
Quarantined: record.Quarantined,
164+
Created: record.Created,
165+
Updated: record.Updated,
166+
Isolation: record.Isolation,
167+
ReconnectOnUse: record.ReconnectOnUse,
168+
LauncherWaitTimeout: record.LauncherWaitTimeout,
166169
})
167170
}
168171

internal/storage/models.go

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -68,22 +68,23 @@ const CurrentSchemaVersion = 2
6868

6969
// UpstreamRecord represents an upstream server record in storage
7070
type UpstreamRecord struct {
71-
ID string `json:"id"`
72-
Name string `json:"name"`
73-
URL string `json:"url,omitempty"`
74-
Protocol string `json:"protocol,omitempty"` // stdio, http, sse, streamable-http, auto
75-
Command string `json:"command,omitempty"`
76-
Args []string `json:"args,omitempty"`
77-
WorkingDir string `json:"working_dir,omitempty"` // Working directory for stdio servers
78-
Env map[string]string `json:"env,omitempty"`
79-
Headers map[string]string `json:"headers,omitempty"` // For HTTP authentication
80-
OAuth *config.OAuthConfig `json:"oauth,omitempty"` // OAuth configuration
81-
Enabled bool `json:"enabled"`
82-
Quarantined bool `json:"quarantined"` // Security quarantine status
83-
Created time.Time `json:"created"`
84-
Updated time.Time `json:"updated"`
85-
Isolation *config.IsolationConfig `json:"isolation,omitempty"` // Per-server isolation settings
86-
ReconnectOnUse bool `json:"reconnect_on_use,omitempty"` // Attempt reconnection on tool call
71+
ID string `json:"id"`
72+
Name string `json:"name"`
73+
URL string `json:"url,omitempty"`
74+
Protocol string `json:"protocol,omitempty"` // stdio, http, sse, streamable-http, auto
75+
Command string `json:"command,omitempty"`
76+
Args []string `json:"args,omitempty"`
77+
WorkingDir string `json:"working_dir,omitempty"` // Working directory for stdio servers
78+
Env map[string]string `json:"env,omitempty"`
79+
Headers map[string]string `json:"headers,omitempty"` // For HTTP authentication
80+
OAuth *config.OAuthConfig `json:"oauth,omitempty"` // OAuth configuration
81+
Enabled bool `json:"enabled"`
82+
Quarantined bool `json:"quarantined"` // Security quarantine status
83+
Created time.Time `json:"created"`
84+
Updated time.Time `json:"updated"`
85+
Isolation *config.IsolationConfig `json:"isolation,omitempty"` // Per-server isolation settings
86+
ReconnectOnUse bool `json:"reconnect_on_use,omitempty"` // Attempt reconnection on tool call
87+
LauncherWaitTimeout config.Duration `json:"launcher_wait_timeout,omitempty"` // Spec 046: max wait for locally-launched HTTP/SSE upstream URL to become reachable
8788
}
8889

8990
// ToolStatRecord represents tool usage statistics

internal/upstream/launcher/launcher.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,15 @@ func (h *handle) stopLocked(ctx context.Context) error {
266266
}
267267

268268
func (h *handle) reap() {
269-
err := h.cmd.Wait()
270-
// Drain log pumps so any final output lands in the sink.
269+
// Drain log pumps FIRST. exec.Cmd.Wait() closes the parent-side stdout/stderr
270+
// pipes as part of its cleanup; if we call Wait() before the pumps have read
271+
// to EOF, the pump scanners can race against that close and observe EOF with
272+
// buffered child output still in flight, dropping it on the floor. Per the
273+
// exec.Cmd doc: "it is thus incorrect to call Wait before all reads from the
274+
// pipe have completed." Pumps naturally finish when the child exits and the
275+
// kernel propagates EOF — we don't need Wait() to reach that state.
271276
h.pumpWG.Wait()
277+
err := h.cmd.Wait()
272278

273279
h.waitErrMu.Lock()
274280
h.waitErr = err

internal/upstream/launcher/wait_test.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,13 @@ func TestWaitForURL_InfersDefaultPort(t *testing.T) {
127127
// that an http:// URL without a port is parsed (rather than rejected)
128128
// by checking it produces a "not reachable" error rather than a
129129
// parse error within the short timeout window.
130+
//
131+
// Use TEST-NET-1 (RFC 5737, 192.0.2.0/24) as the host — it's reserved
132+
// for documentation and guaranteed non-routable, so the dial fails
133+
// consistently across OSes. 127.0.0.1:80 is bound on Windows GitHub
134+
// runners (IIS/WinNAT), which would make this test pass spuriously.
130135
ctx := context.Background()
131-
err := WaitForURL(ctx, "http://127.0.0.1/", 100*time.Millisecond)
136+
err := WaitForURL(ctx, "http://192.0.2.1/", 100*time.Millisecond)
132137
require.Error(t, err)
133138
assert.Contains(t, err.Error(), "not reachable", "default-port inference should reach the polling loop, not fail at parse")
134139
}

oas/docs.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

oas/swagger.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,14 @@ components:
500500
type: object
501501
isolation:
502502
$ref: '#/components/schemas/config.IsolationConfig'
503+
launcher_wait_timeout:
504+
description: |-
505+
LauncherWaitTimeout caps how long mcpproxy will wait for a locally-launched
506+
HTTP/SSE upstream's URL to become reachable after Spawn(). Only consulted
507+
when the server is configured with both Command and an HTTP/SSE URL — i.e.,
508+
mcpproxy starts the process AND connects via network. Stdio servers ignore
509+
this field. Zero or unset → 30s default.
510+
type: string
503511
name:
504512
type: string
505513
oauth:

0 commit comments

Comments
 (0)