-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix: resolve workspace materialization race on Windows (#671) #690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c9bea3f
c336d9c
722eefb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -349,6 +349,15 @@ async def _run_cycle( # noqa: PLR0912, PLR0915 | |
| while True: | ||
| try: | ||
| await coordinator.mark_running(agent_id) | ||
|
|
||
| # --- PRODUCTION FIX FOR AWS BEDROCK TOOL_CHOICE COMPLIANCE --- | ||
| if hasattr(run_config, "extra_body") and isinstance(run_config.extra_body, dict): | ||
| if "tool_choice" in run_config.extra_body: | ||
| run_config.extra_body["tool_choice"] = {"type": "auto"} | ||
| elif hasattr(run_config, "tool_choice") and (run_config.tool_choice == "auto" or isinstance(run_config.tool_choice, str)): | ||
| run_config.tool_choice = {"type": "auto"} | ||
|
Comment on lines
+357
to
+358
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/core/execution.py
Line: 357-358
Comment:
**Tool choice overwritten** This branch still rewrites every string `run_config.tool_choice`, not only the Bedrock-incompatible `"auto"` value. When a caller configures `"required"` or `"none"`, the shared `RunConfig` is mutated to `{"type": "auto"}` before `Runner.run_streamed()`, so later cycles and child agents can run with automatic tool selection instead of the configured policy.
How can I resolve this? If you propose a fix, please make it concise. |
||
| # ------------------------------------------------------------- | ||
|
|
||
| stream = Runner.run_streamed( | ||
| agent, | ||
| input=input_data, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ | |
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import os | ||
| import sys | ||
| from collections.abc import Awaitable, Callable | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
|
|
@@ -47,11 +49,46 @@ async def _docker_backend( | |
|
|
||
| from strix.runtime.docker_client import StrixDockerSandboxClient | ||
|
|
||
| # Handle Issue #671 Edge Cases | ||
| concurrency_limits = None | ||
| default_limit = 1 if sys.platform == "win32" else None | ||
| limit_val = os.environ.get("STRIX_DOCKER_CONCURRENCY", default_limit) | ||
|
|
||
| if limit_val is not None: | ||
| try: | ||
| # Prevents <= 0 limits breaking the SDK validation | ||
| limit = max(1, int(limit_val)) | ||
|
|
||
| # Try primary import path, fallback if SDK changes | ||
| try: | ||
| from agents.sandbox.artifacts import SandboxConcurrencyLimits | ||
| except ImportError: | ||
| from agents.sandbox import SandboxConcurrencyLimits | ||
|
Comment on lines
+63
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/runtime/backends.py
Line: 63-66
Comment:
**Wrong import path** The concurrency limit type is still loaded from SDK surfaces that do not export it in the pinned API. On the default Windows path, both import attempts can fail before the Docker backend starts. If a Windows user sets `STRIX_DOCKER_CONCURRENCY` to another value, the same import failure is swallowed and the backend continues with `concurrency_limits=None`, so workspace materialization can still run concurrently.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| concurrency_limits = SandboxConcurrencyLimits( | ||
| manifest_entries=limit, | ||
| local_dir_files=limit | ||
| ) | ||
| logger.debug(f"Applied SandboxConcurrencyLimits: {limit}") | ||
| except (ImportError, ValueError) as e: | ||
| if sys.platform == "win32" and limit_val == 1: | ||
| # Fail loudly on Windows to avoid silent race condition | ||
| raise RuntimeError("Failed to import SandboxConcurrencyLimits required for Windows execution.") from e | ||
| logger.warning("Invalid STRIX_DOCKER_CONCURRENCY or import failed. Proceeding with defaults.") | ||
|
|
||
| client = StrixDockerSandboxClient(docker.from_env()) | ||
| client.strix_bind_mounts = bind_mounts or [] | ||
| options = DockerSandboxClientOptions(image=image, exposed_ports=exposed_ports) | ||
|
|
||
| # Create the session (without concurrency_limits keyword) | ||
| session = await client.create(options=options, manifest=manifest) | ||
| await session.start() | ||
|
|
||
| # Pass concurrency_limits where the session is actually started/materialized | ||
| start_kwargs = {} | ||
| if concurrency_limits is not None: | ||
| start_kwargs["concurrency_limits"] = concurrency_limits | ||
|
|
||
| await session.start(**start_kwargs) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/runtime/backends.py
Line: 91
Comment:
**Start rejects kwargs** Passing `concurrency_limits` to `session.start()` still targets the wrong SDK call. The Docker sandbox session `start()` method accepts no keyword arguments, so any path that successfully builds `start_kwargs` fails backend startup with an unexpected-keyword error before workspace materialization can run.
How can I resolve this? If you propose a fix, please make it concise. |
||
| return client, session | ||
|
|
||
|
|
||
|
|
@@ -90,4 +127,4 @@ def register_backend(name: str, backend: SandboxBackend) -> None: | |
|
|
||
|
|
||
| def supported_backends() -> list[str]: | ||
| return sorted(_BACKENDS) | ||
| return sorted(_BACKENDS) | ||
Uh oh!
There was an error while loading. Please reload this page.