Skip to content

Fix: poll agent engine operations without a trailing sleep - #777

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/lro-poll-trailing-sleep
Open

Fix: poll agent engine operations without a trailing sleep#777
AmaadMartin wants to merge 3 commits into
mainfrom
fix/lro-poll-trailing-sleep

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 7, 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: Two long-running-operation poll loops race the poll against a fixed sleep with Promise.all, so every iteration costs max(poll, interval). The iteration that observes done still pays its own sleep. VertexAiSessionService.createSession() therefore blocks about 1000 ms after the session already exists, and adk deploy agent_engine blocks about 5000 ms after the Reasoning Engine already exists.

Solution: Sleep between polls instead of alongside them. The loop now delays only when attempts > 0, so the first poll still fires immediately and the poll that observes done returns with no timer pending. The interval moves into a named constant in each file (POLL_INTERVAL_MS, 1000 ms in core and 5000 ms in dev). The attempt budget stays at 30 polls and the timeout error strings are byte-identical, so the worst case only loses the trailing sleep (30 polls separated by 29 sleeps).

Collision check: I listed all 100 open PRs on the fork and read the diffs of every PR that touches either file (#759, #596, #590, #586) plus the session-service PRs (#503, #474, #657, #563, #512, #622). None of them changes a poll loop. No overlap, so this branches from main.

The two other hand-rolled poll loops in core/src/code_executors/agent_engine_sandbox_code_executor.ts have a different defect (they sleep before the first poll) and are out of scope here.

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.

Four new tests, two per loop. Each pair pins both halves of the schedule: the first poll settles the promise with the fake clock still at its start value and vi.getTimerCount() === 0, and the never-done path still issues exactly 30 polls separated by 29 interval sleeps before it throws the unchanged timeout error.

One change to existing tests, in its own commit. The two deploy timeout tests attached their expect(...).rejects assertion after the 30-iteration timer advance loop. The loop now drops its trailing sleep, so the deploy rejects one advance earlier and Node reported the rejection as unhandled; the first CI run failed on that with 3140 tests passing and Errors 1 error. I moved the assertion above the advance loop and awaited it after. The assertion, the matcher and the expected message are unchanged. No test was skipped, weakened or deleted.

Commands run on the pushed commit:

npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts   # 64 passed
npx vitest run --project unit:dev dev/test/cli/cli_deploy_agent_engine_test.ts            # 20 passed
npx vitest run --project integration tests/integration/sessions/vertex_ai_session_service_test.ts  # 3 passed
npm run build      # exit 0
npm run lint       # exit 0
npm run format:check  # exit 0

npm run ts:check reports the same pre-existing errors before and after this branch (diff of the two runs is empty). This change adds none.

CI is green on all four legs. The macOS leg first failed on the known tests/integration/app_loader/app_loader_test.ts discovery timeout, which also fails on an unmodified-main control run and is unrelated to this change; it passed on retry.

Measured dead latency, core/test/sessions/vertex_ai_session_service_test.ts:

test time file duration wall clock
before 6092 ms 13.53 s 15.02 s
after 65 ms 7.27 s 8.76 s

Six createSession tests each took about 1002 ms before, because the default createInternal mock has no done field and those tests do not fake timers. They now take about 1 ms each. The dev file was already fast because its beforeEach stubs setTimeout to run synchronously.

Coverage of the changed lines is 100%. Both sides of attempts > 0 and both loop exits are exercised: the v8 report lists no uncovered statement in core/src/sessions/vertex_ai_session_service.ts:185-192 or dev/src/cli/deploy/cli_deploy_agent_engine.ts:182-193.

Mutation testing. I restored the Promise.all race in each file in turn and re-ran the suite. All four new tests failed:

  • Core test 1: AssertionError: expected false to be true at expect(resolved).toBe(true). The create is parked on the trailing 1000 ms timer.
  • Core test 2: AssertionError: expected 1 to be +0 at expect(vi.getTimerCount()).toBe(0). The unfixed loop leaves the 30th sleep pending. The poll-count assertions still passed here, so getTimerCount() is the assertion that carries the signal.
  • Dev test 3: Error: Test timed out in 5000ms. The deploy never settles inside its budget.
  • Dev test 4: AssertionError: expected "spy" to be called 30 times, but got 4 times.

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 GCP project and an Agent Engine, so the fake-clock tests above are the evidence. To reproduce by hand:

  1. Point VertexAiSessionService at a sessions stub whose createInternal resolves {name: 'operations/op-1', done: false} and whose getSessionOperationInternal resolves {done: true, response: {...}}.
  2. Time one createSession({appName: '12345', userId: 'u'}) call.
  3. Before: about 1000 ms. After: about 0 ms.

The deploy path reproduces the same way against client.agentEnginesInternal.getAgentOperationInternal, with 5000 ms instead of 1000 ms.

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 7, 2026 07:56
Both LRO poll loops raced the poll against a fixed sleep with Promise.all,
so each iteration cost max(poll, interval). The iteration that observed
done still paid its own sleep, adding 1000 ms to every createSession and
5000 ms to every agent engine deploy.

Sleep between polls instead. The first poll still fires immediately, the
attempt budget is still 30 polls, and the timeout error strings are
unchanged.
Four tests: the first poll must settle the promise with the clock still at
its start value and no pending timer, and the not-done path must still
issue 30 polls separated by 29 interval sleeps before it times out.
…mers

Both agent engine deploy timeout tests attached their rejects assertion
after the timer advance loop. The loop now drops its trailing sleep, so
the deploy rejects one advance earlier and Node reported the rejection as
unhandled, which failed the run under load. The assertion and its message
are unchanged.
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