Uh oh!
There was an error while loading. Please reload this page.
fix(engine): stop non-LLM steps constructing a provider for a context window - #456
Merged
Merged
Conversation
… window `_execute_loop` built every `agent_started` payload with a `context_window_max` resolved through `_get_context_window_for_agent`, which resolves a provider -- and the registry builds providers lazily. A `wait` / `set` / `script` / `terminate` / `human_gate` step has no model, so that construction bought a guaranteed `None`. It is also charged to `limits.timeout_seconds`, because it runs inside the engine's timed loop. Measured on a provider-free wait workflow, the first step spent 0.434s of the budget building a Copilot SDK client; on a cold Windows CI runner it consumed enough of `examples/wait-smoke.yaml`'s 3s cap to time the workflow out, exit the child non-zero, and fail the `Web BG Smoke (windows-latest)` job as a launcher failure (3 of the 4 runs since that job was added). In-engine time for that workflow drops from 1.46s to 1.01s, of which 1.00s is the sleeps it is meant to perform. `is_llm_agent` was already computed two lines above, so the fix is to gate the lookup on it. Provider-backed agents are unaffected; the completion-side sites already run only in the LLM branches. Also fixed, both surfaced by the same CI failures: - `test_a_scan_failure_does_not_wedge_the_screen` and its render-failure sibling asserted `_refreshing is False` a single instant after `settle()`, while the test's own 0.05s poll timer legitimately flips the flag back to `True` -- including inside `settle()`'s trailing pause. That failed ~20% of local runs and once on CI (Python 3.13, ubuntu). They now wait for the flag to *clear*, which is what the `finally` actually guarantees: without it the flag latches `True` forever and the wait times out, so the regression is still caught. 20/20 clean, from 2/10 failing. - `examples/wait-smoke.yaml`'s 3s cap left no headroom for a cold Windows runner even after the engine fix, so it is now 15s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two failing jobs on
main:Web BG Smoke (windows-latest)(failing 3 of the 4 runs since it was added in #450) and a flakyTest (Python 3.13, ubuntu-latest).1. A step with no model was constructing a provider
_execute_loopbuilt everyagent_startedpayload with acontext_window_maxresolved through_get_context_window_for_agent, which resolves a provider — andProviderRegistrybuilds providers lazily. Await/set/script/terminate/human_gatestep has no model, so that construction bought a guaranteedNone.It is also charged to
limits.timeout_seconds, because it runs inside the engine's timed loop. Profiling a provider-free wait workflow, the first step spent 0.434s of the budget building a Copilot SDK client:On a cold Windows CI runner that (plus general per-step overhead) consumed enough of
examples/wait-smoke.yaml's 3s cap to time the workflow out. The child then exits non-zero, and the launch gate reports it asBackground process exited before the workflow started (code 1)— failing the smoke job for something it does not test. In-engine time for that workflow drops from 1.46s to 1.01s, of which 1.00s is the sleeps it is meant to perform.is_llm_agentwas already computed two lines above, so the fix gates the lookup on it. Provider-backed agents are unaffected; the two completion-side call sites already run only in the LLM branches.TestNonLlmStepsSkipTheProviderLookuppins it with a spy onProviderRegistry.get_provider— verified to fail on the unfixed engine, so it is not vacuous.2.
test_a_scan_failure_does_not_wedge_the_screenwas racing its own poll timerThe two
TestRunsScreenGuardRecoverytests asserted_refreshing is Falsea single instant aftersettle(), while shrinkingPOLL_INTERVAL_SECONDSto 0.05s — so a tick landing duringsettle()'s trailingpilot.pause()legitimately set the flag back toTruebefore the assertion read it (assert True is False). Reproduced locally at ~20%.They now wait for the flag to clear, which is what the
finallyactually guarantees: without it the flag latchesTruefor the session and the wait times out, so the regression is still caught. 20/20 clean, from 2/10 failing.3. Headroom for the smoke fixture
examples/wait-smoke.yaml's 3s cap left no margin on a cold Windows runner even after fix 1, so it is now 15s, withexamples/README.mdand theci.ymljob comment updated. The timeout path the example demonstrates is unchanged — drive it with a larger--input middle_duration_ms.Validation
make check(ruff, ruff format, ty) — cleantests/test_engine+tests/test_fleet— 1602 passed, 1 skippedmake validate-examples— exit 0conductor run examples/wait-smoke.yaml --web-bg— launches, reachesworkflow_completedin 1.013s in-engine,conductor stopcleans up