Repository navigation
fix(agentmesh/sandbox): honour SandboxConfig.TimeoutSeconds in docker exec - #4234
PratikDhanave (PratikDhanave) wants to merge 1 commit into
Conversation
… exec ExecuteCode always used the fixed dockerExecTimeout (60s) and the provider never stored the per-session config, so SandboxConfig.TimeoutSeconds was a dead field — the const-block comment even claimed docker exec "falls back to SandboxConfig.TimeoutSeconds when set", which was never wired. The TypeScript port already fixed this (DockerSandboxProvider.executeCode); the Go port lagged. - Track a per-session exec deadline (resolveExecTimeout(config.TimeoutSeconds)) in CreateSession, consume it in ExecuteCode, and clean it up in DestroySession alongside the container entry. - resolveExecTimeout guards non-positive/NaN/Inf values, falling back to dockerExecTimeout so a misconfigured session can't disable the deadline. Default config is 60s, so default behavior is unchanged. - Add TestResolveExecTimeout (table; Docker-free). The end-to-end exec path is covered by the existing integration test, which skips without Docker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Pratik Dhanave <i.pratikdhanave@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🟡 Contributor Check: MEDIUM
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:MEDIUM
|
The fix checks out: ExecuteCode used a fixed 60 second timeout and the provider never kept the config; the per-session timeout map mirrors the TypeScript sandbox and is cleaned up in DestroySession and the eviction loop, and non-positive values fall back to the default. go build, go vet and go test -race pass (350 tests), gofmt is clean on both files. One cosmetic item: the rewritten comment at sandbox.go line 35 uses two U+201C curly quotes around docker exec; plain backticks or ASCII quotes please. Before I approve, please replace the description with the repository PR template and complete it, in particular Type of Change, Packages Affected, Checklist and the AI Assistance attestations, since the commit records Claude Code as co-author. Same ask as on #4226. |
This branch has not been deployed
2 participants
Summary
DockerSandboxProvider.ExecuteCodealways used the fixeddockerExecTimeout(60s) and the provider never stored the per-session config, soSandboxConfig.TimeoutSecondswas a dead field. The const-block comment even claimeddocker exec"falls back toSandboxConfig.TimeoutSecondswhen set" — a behavior that was never wired.The TypeScript port already fixed exactly this (
DockerSandboxProvider.executeCode, which carries the note "Previously this was hard-coded to 60_000 ms, so a custom timeoutSeconds was silently ignored"); the Go port lagged.Change
resolveExecTimeout(config.TimeoutSeconds)) inCreateSession, consume it inExecuteCode, and clean it up inDestroySessionalongside the container entry.resolveExecTimeoutguards non-positive / NaN / Inf values, falling back todockerExecTimeoutso a misconfigured session can't disable the deadline. Default config is 60s → default behavior unchanged.Verification
TestResolveExecTimeout(table; Docker-free) covers custom/fractional honored + zero/negative/NaN/Inf fallback.go build ./...,go vet, andgo test -race ./packages/agentmesh/pass. The end-to-end exec path is covered by the existing integration test (skips without Docker).🤖 Generated with Claude Code