Skip to content

fix(typescript): honor configured sandbox timeout in executeCode#3122

Closed
karimbaidar wants to merge 1 commit into
microsoft:mainfrom
karimbaidar:fix/sandbox-timeout-3118
Closed

fix(typescript): honor configured sandbox timeout in executeCode#3122
karimbaidar wants to merge 1 commit into
microsoft:mainfrom
karimbaidar:fix/sandbox-timeout-3118

Conversation

@karimbaidar

@karimbaidar karimbaidar commented Jun 20, 2026

Copy link
Copy Markdown

Description

DockerSandboxProvider lets callers pass a custom SandboxConfig.timeoutSeconds, but that value was silently ignored at execution time:

  • createSession() applied the memory/CPU/network/env settings from the config but never stored the config object, so the per-session timeout was discarded after the container was created.
  • executeCode() then ran the code with a hardcoded { timeout: 60_000 }.

As a result, the sandbox's execution-time limit was always 60s regardless of configuration. A caller who set timeoutSeconds: 2 got no enforcement below 60s, and a caller who set a larger value (e.g. 300) had work killed at 60s. Since the timeout is a sandbox containment control, this gave a false sense of control over how long agent-generated code can run.

Fix

  • Persist the resolved config per session in createSession() (a sessionId -> SandboxConfig map mirroring the existing containers map).
  • Use cfg.timeoutSeconds * 1000 for the docker exec timeout in executeCode(), falling back to defaultSandboxConfig() if a session somehow has no stored config.
  • Drop the stored config in destroySession() so the new map does not leak.

The change is non-breaking: the public method signatures are unchanged, and the default config keeps the previous 60s behavior.

Verification

  • Added tests/sandbox-timeout.test.ts, which stubs child_process (no Docker needed) and asserts the configured timeout reaches docker exec. It fails against the old code (expected 2000, received 60000) and passes with the fix.
  • Existing sandbox tests pass; npm run build and npm run lint are clean.
  • Confirmed end-to-end against real Docker: with timeoutSeconds: 2, a time.sleep(5) snippet is now cut off at ~2s, whereas on main it ran the full ~5s and printed its output.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Maintenance (dependency updates, CI/CD, refactoring)
  • Security fix

Package(s) Affected

  • agent-os-kernel
  • agent-mesh
  • agent-runtime
  • agent-sre
  • agent-governance
  • docs / root

Checklist

  • My code follows the project style guidelines (ruff check)
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass (pytest)
  • I have updated documentation as needed
  • I have signed the Microsoft CLA

Attribution & Prior Art

  • This contribution does not contain code copied or derived from other projects without attribution
  • Any external projects that inspired this design are credited in code comments or documentation
  • If this PR implements functionality similar to an existing open-source project, I have listed it below

Prior art / related projects (if any):

Not applicable — this wires an existing SandboxConfig field through to execution; no external code or design was adapted.

AI Assistance

  • I can explain every meaningful change in this PR: what it does, why, and what tradeoffs were considered
  • I have run tests and verification appropriate for this change
  • No part of this PR was autonomously submitted by an AI agent without my review
  • I have not used AI to generate review comments on others' PRs

If AI tools materially shaped this change, briefly note what was used:

An AI coding assistant was used for drafting and test scaffolding. All output was reviewed and validated locally (unit test plus a live Docker reproduction) before submission.

IP, Patents, and Licensing

  • This contribution does not implement patent-pending or patent-encumbered techniques
  • This contribution does not require an NDA or licensing agreement to understand or use
  • Any AI tools used have terms compatible with the MIT License

Related Issues

Closes #3118

fix(typescript): honor configured sandbox timeout in executeCode
535c103 
executeCode hardcoded a 60s timeout and createSession never stored the session config, so SandboxConfig.timeoutSeconds was silently ignored. Persist the config per session and apply timeoutSeconds when executing code.

Closes microsoft#3118

Signed-off-by: Karim Baidar <karimbaidar@gmail.com>
@github-actions github-actions Bot added tests size/M Medium PR (< 200 lines) labels Jun 20, 2026
@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: contributor-guide — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Welcome, and thank you for your detailed contribution!

Great job on identifying and fixing the timeout enforcement issue while adding thorough test coverage.

Before merging:

  1. Ensure all tests pass (pytest is unchecked in the checklist).
  2. Confirm if any documentation updates are needed for this change.

For guidance, see CONTRIBUTING.md. Let us know if you need help!

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

No breaking changes detected.

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: security-scanner — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

No security issues found.

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

  • DockerSandboxProvider.createSession() in agent-governance-typescript/src/sandbox.ts -- missing docstring
  • DockerSandboxProvider.executeCode() in agent-governance-typescript/src/sandbox.ts -- missing docstring
  • DockerSandboxProvider.destroySession() in agent-governance-typescript/src/sandbox.ts -- missing docstring
  • README.md -- no update for the behavioral change in sandbox timeout handling
  • CHANGELOG.md -- missing entry for the fix to honor SandboxConfig.timeoutSeconds in executeCode()

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 0 blockers, 1 warning. Fix addresses a critical security issue effectively, but additional test coverage is recommended.

# Sev Issue Where
1 Warn Limited test coverage for edge cases tests/sandbox-timeout.test.ts

Action items:

  1. None.

Warnings:

# Issue Where Follow-up
1 Add tests for edge cases like invalid or extreme timeout values (e.g., negative, zero, very large). tests/sandbox-timeout.test.ts Fine as follow-up PR.

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
🤖 AI Agent: test-generator — `agent-governance-typescript/src/sandbox.ts`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

agent-governance-typescript/src/sandbox.ts

  • test_destroySession_clears_config_map -- Verify that destroySession() removes the session's configuration from the configs map.
  • test_executeCode_no_session_config -- Test executeCode() behavior when a session ID is invalid or has no associated configuration.

agent-governance-typescript/tests/sandbox-timeout.test.ts

  • test_executeCode_timeout_error -- Validate that executeCode() correctly handles a timeout error (e.g., when the execution exceeds the configured timeout).

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown

PR Review Summary

Check Status Details
🔍 Code Review ⚠️ Missing No current-run comment
🛡️ Security Scan ⚠️ Missing No current-run comment
🔄 Breaking Changes ⚠️ Missing No current-run comment
📝 Docs Sync ⚠️ Missing No current-run comment
🧪 Test Coverage ⚠️ Missing No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

@karimbaidar

karimbaidar commented Jun 20, 2026

Copy link
Copy Markdown
Author

Closing this in favor of #3120, which fixes the same issue in a better way.

It runs timeout inside the container, so code that runs over the limit is actually killed (and the timeout is reported via exit code 124). Mine only changed the Node-side exec timeout, which returns after the limit but leaves the process still running inside the container. #3120 is the right approach, so going with that one.

@karimbaidar karimbaidar mentioned this pull request Jun 20, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MohammadHaroonAbuomar MohammadHaroonAbuomar Awaiting requested review from MohammadHaroonAbuomar MohammadHaroonAbuomar is a code owner

@imran-siddique imran-siddique Awaiting requested review from imran-siddique imran-siddique is a code owner

Assignees

No one assigned

Labels

size/M Medium PR (< 200 lines) tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: TypeScript Docker sandbox ignores custom timeoutSeconds - SandboxConfig.timeoutSeconds

1 participant

@karimbaidar