Conversation
Add GetEnvDuration to envutil, consistent with GetEnvString/GetEnvInt/GetEnvBool. Use it in server/transport.go to make the unified-mode session timeout configurable via MCP_GATEWAY_SESSION_TIMEOUT (default: 2h). Previously the 2h timeout was hardcoded. Operators with tighter security requirements or faster workflows can now shorten it without recompiling. Format: any string accepted by time.ParseDuration, e.g. '30m', '1h', '4h'. Invalid/zero/negative values fall back to the 2h default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new environment helper to parse time.Duration values and uses it to make the MCP Streamable HTTP session timeout configurable via MCP_GATEWAY_SESSION_TIMEOUT (default remains 2h).
Changes:
- Add
envutil.GetEnvDuration(patterned after existing envutil helpers). - Wire
MCP_GATEWAY_SESSION_TIMEOUTintosdk.StreamableHTTPOptions.SessionTimeout. - Add table-driven unit tests for duration parsing and fallback behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/envutil/envutil.go | Introduces GetEnvDuration for parsing positive durations from env vars. |
| internal/server/transport.go | Uses GetEnvDuration to configure MCP session timeout via MCP_GATEWAY_SESSION_TIMEOUT. |
| internal/envutil/envutil_test.go | Adds unit tests covering valid/invalid duration env values and a session-timeout scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetEnvDuration returns the time.Duration value of the environment variable specified by envKey. | ||
| // If the environment variable is not set, is empty, or cannot be parsed by time.ParseDuration, | ||
| // it returns the defaultValue. Accepts any string valid for time.ParseDuration (e.g. "2h", "30m", "90s"). | ||
| func GetEnvDuration(envKey string, defaultValue time.Duration) time.Duration { | ||
| if envValue := os.Getenv(envKey); envValue != "" { | ||
| if d, err := time.ParseDuration(envValue); err == nil && d > 0 { | ||
| return d |
There was a problem hiding this comment.
The docstring says this falls back only when the env var is unset/empty or cannot be parsed, but the implementation also falls back when the parsed duration is non-positive (d <= 0). Please update the comment to mention the > 0 requirement (similar to GetEnvInt) so behavior and docs match.
| t.Run(tt.name, func(t *testing.T) { | ||
| os.Unsetenv(tt.envKey) | ||
| defer os.Unsetenv(tt.envKey) | ||
|
|
||
| if tt.setEnv { | ||
| os.Setenv(tt.envKey, tt.envValue) | ||
| } | ||
|
|
There was a problem hiding this comment.
Tests are manually setting/unsetting environment variables via os.Unsetenv/os.Setenv. The repo frequently uses testing.T.Setenv for this (e.g., internal/sys/container_test.go:12-68), which automatically restores env vars via t.Cleanup and is safer if tests ever become parallelized. Consider switching these cases to t.Setenv (and avoiding manual Unsetenv/defer).
🤖 This PR was created by Repo Assist, an automated AI assistant.
Summary
Adds
GetEnvDurationtointernal/envutil(consistent with existingGetEnvString/GetEnvInt/GetEnvBool), and uses it to make the unified-mode MCP session timeout configurable via theMCP_GATEWAY_SESSION_TIMEOUTenvironment variable.Motivation
The session timeout in
server/transport.gowas hardcoded at 2 hours with a comment "2h accommodates long-running workflows with idle periods". While the default is good, operators may need to:This was identified as a best-practice alignment item in the go-sdk module review (#3054).
Changes
internal/envutil/envutil.gointernal/server/transport.gointernal/envutil/envutil_test.go9 table-driven test cases covering: valid durations ("2h", "30m", "90s", "1h30m"), missing env var, empty string, invalid string, zero duration, and negative duration. Plus a real-world scenario test for
MCP_GATEWAY_SESSION_TIMEOUT.Usage
Invalid, zero, or negative values fall back to the 2h default silently.
Test Status
make agent-finishedcould not be run. The changes are:gofmtclean on all changed files)GetEnvDurationimplementation follows the identical pattern asGetEnvInt(which is well-tested and in production use)