Skip to content

Fix: poll Agent Engine and sandbox operations before sleeping - #872

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/sandbox-executor-leading-poll-sleep
Open

Fix: poll Agent Engine and sandbox operations before sleeping#872
AmaadMartin wants to merge 3 commits into
mainfrom
fix/sandbox-executor-leading-poll-sleep

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

  2. Or, if no issue exists, describe the change:

Problem: The two long-running-operation loops in AgentEngineSandboxCodeExecutor sleep 1000 ms before their first status poll. createInternal returns a pending operation, so the loop is always taken and the executor cannot see a finished operation for a full second. An executor built with neither agentEngineResourceName nor sandboxResourceName runs both loops, so the first executeCode pays a minimum of 2000 ms of dead time.

Solution: Sleep between polls instead of before them. Each loop now delays only when attempts > 0, so the first poll fires immediately and the poll that observes done returns with no timer pending. The interval moves into a module-level POLL_INTERVAL_MS constant.

The bound is unchanged: DEFAULT_MAX_ATTEMPTS (180) still counts status polls, and both timeout error strings are byte-identical. As a direct consequence each loop now performs 180 polls separated by 179 sleeps instead of 180, so the worst-case wall clock before a timeout is one interval shorter per loop. That is the intended meaning of "sleep between polls".

I considered extracting one shared poll helper for the two loops and did not. The two responses come from different SDK methods with different declared types, so a generic helper needs a type parameter and a done-bearing constraint; getting that wrong is how any or as never enters the diff. The in-place edit changes no types at all.

The same defect exists in core/src/sessions/vertex_ai_session_service.ts and dev/src/cli/deploy/cli_deploy_agent_engine.ts. Those are a separate change and this PR does not touch them.

Collision check: I listed all 400 open PRs on the fork and scanned every one for a diff touching agent_engine_sandbox_code_executor.ts. Five touch either file (#497, #514, #565, #678, #806) and none changes a poll loop; their hunks are disjoint from mine. #777 fixes the trailing sleep in the two sibling files and states the code-executor loops are out of its scope. So this branches from main.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Three new tests in core/test/code_executors/agent_engine_sandbox_code_executor_test.ts, 26 tests before and 29 after. No existing test was edited, skipped, weakened or deleted; the only changed line is the vitest import, which now also imports afterEach for a shared vi.useRealTimers() hook.

  • Test 1 makes both create calls return a pending operation and leaves both operation getters at their done: true defaults. It drains microtasks with vi.advanceTimersByTimeAsync(0) and asserts vi.getTimerCount() === 0, then that executeCode resolves after one poll per loop.
  • Tests 2 and 3 hold one loop's operation at done: false and step the fake clock. Each asserts the first poll lands at 0 ms, no second poll at POLL_INTERVAL_MS - 1, a second poll at POLL_INTERVAL_MS, and exactly 180 polls before the unchanged timeout error.

Commands run on the pushed commit:

npx vitest run --project unit:core core/test/code_executors/agent_engine_sandbox_code_executor_test.ts   # 29 passed
npm run build          # exit 0
npm run lint           # exit 0
npm run format:check   # exit 0

npm run ts:check reports Found 287 errors in 42 files both on this branch and with the two files restored from main. Neither changed file appears in that list, so this change adds no type error.

Coverage of the changed file is 99.64% statements and 98.57% branches. Both sides of both new attempts > 0 guards are exercised, so the new lines and new branches are at 100%. The one uncovered line is 84, the default: throw of the unsupported-language switch, which is untouched by this change and uncovered on main as well.

Mutation testing. Each mutation was applied, the suite was re-run, and the source was restored:

  1. Restored the unconditional sleep in the agent engine loop. Test 1 failed with AssertionError: expected 1 to be +0 at expect(vi.getTimerCount()).toBe(0), and Test 2 failed with expected "spy" to be called 1 times, but got 0 times.
  2. Restored the unconditional sleep in the sandbox loop. Test 1 failed the same way and Test 3 failed with expected "spy" to be called 1 times, but got 0 times.
  3. Inverted the engine guard to if (attempts >= 0). Tests 1 and 2 failed with the same two messages, so the tests pin the condition and not merely the presence of an if.
  4. Changed the sandbox interval to 900 ms. Test 3 failed with expected "spy" to be called 1 times, but got 2 times at the POLL_INTERVAL_MS - 1 step.

CI is green on all four run-tests legs. The Windows leg first failed on core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout with Test timed out in 5000ms. That test spawns a real subprocess, this change does not touch it, and the same leg passed on the previous commit; it passed on re-run.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

A real reproduction needs a live Vertex AI project, so the fake-clock tests above are the evidence. With a project available: set GOOGLE_CLOUD_PROJECT, build AgentEngineSandboxCodeExecutor with no resource names, and time one executeCode. The cold-path wall clock drops by about 2 seconds and the result payload is identical.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 9, 2026 08:38
…sleeping

Both long-running-operation loops slept 1000 ms before their first status
poll, so a cold AgentEngineSandboxCodeExecutor paid 2000 ms of dead time
before it could observe an operation that had already finished. The loops
now delay between polls only. The 180-poll bound and both timeout error
strings are unchanged.
…edule

Three tests: an operation done on its first poll now settles with the fake
clock unmoved and no timer pending, and each loop still spaces its polls by
1000 ms and gives up after 180 polls.
…iable clock assertion

The two-line comment repeated one idea at both poll sites. Date.now() cannot
move under fake timers unless the test advances the clock, so that assertion
pinned nothing; the timer-count check already carries the signal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant