Skip to content

Commit ac0252e

Browse files
CopilotOhYee
andcommitted
fix: use greenlet.error type instead of string check; recreate per thread
- Add GreenletError import with ImportError fallback (same pattern as PlaywrightError) - Replace brittle 'cannot switch to' substring check with dedicated except GreenletError block placed before the generic except Exception, so the correct exception type is matched regardless of message wording - Keep existing sandbox on greenlet error (failure is client-side thread affinity, not a sandbox crash); only reset Playwright and retry - _get_playwright already checks current_thread is not creator_thread (any different thread triggers recreation, not just dead threads) - Update concurrent caching test: each thread now gets its own connection (Playwright Sync API is thread-affine, cross-thread sharing is unsafe) - Replace test_get_playwright_live_thread_not_recreated with test_get_playwright_different_live_thread_recreates_connection to validate the correct per-thread isolation behavior - Update greenlet error tests to use real greenlet.error, verify sandbox is preserved, and check retry behavior Type check: passed (297 source files, no issues) Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
1 parent 915e97a commit ac0252e

File tree

2 files changed

+80
-43
lines changed

2 files changed

+80
-43
lines changed

agentrun/integration/builtin/sandbox.py

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,16 @@ class PlaywrightError(Exception): # type: ignore[no-redef]
2626
pass
2727

2828

29+
try:
30+
from greenlet import error as GreenletError
31+
except ImportError:
32+
33+
class GreenletError(Exception): # type: ignore[no-redef]
34+
"""Fallback greenlet error used when greenlet is not installed."""
35+
36+
pass
37+
38+
2939
class SandboxToolSet(CommonToolSet):
3040
"""沙箱工具集基类
3141
@@ -836,28 +846,25 @@ def _run_in_sandbox(self, callback: Callable[[Sandbox], Any]) -> Any:
836846
"Browser tool-level error (no sandbox rebuild): %s", e
837847
)
838848
return {"error": f"{e!s}"}
839-
except Exception as e:
840-
error_msg = str(e)
841-
if "cannot switch to" in error_msg:
849+
except GreenletError as e:
850+
logger.debug(
851+
"Greenlet thread-binding error, resetting Playwright: %s",
852+
e,
853+
)
854+
# Keep the existing sandbox (it is still healthy); only the
855+
# Playwright connection needs to be recreated on this thread.
856+
try:
857+
self._reset_playwright()
858+
return callback(sb)
859+
except Exception as e2:
842860
logger.debug(
843-
"Greenlet thread-binding error, resetting Playwright: %s",
844-
e,
861+
"Retry after Playwright reset failed: %s",
862+
e2,
845863
)
846-
# Reset only the Playwright connection and keep the existing sandbox
847-
try:
848-
self._reset_playwright()
849-
# Retry once with the same sandbox instance; the original error
850-
# will still be returned if this retry fails.
851-
return callback(sb)
852-
except Exception as e2:
853-
logger.debug(
854-
"Retry after Playwright reset failed: %s",
855-
e2,
856-
)
857-
return {"error": f"{e!s}"}
858-
else:
859-
logger.debug("Unexpected error in browser sandbox: %s", e)
860864
return {"error": f"{e!s}"}
865+
except Exception as e:
866+
logger.debug("Unexpected error in browser sandbox: %s", e)
867+
return {"error": f"{e!s}"}
861868

862869
def _is_infrastructure_error(self, error_msg: str) -> bool:
863870
"""判断是否为基础设施错误 / Check if error is infrastructure-level

tests/unittests/integration/test_browser_toolset_error_handling.py

Lines changed: 54 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -254,35 +254,30 @@ def test_reset_playwright_handles_close_error(self, toolset, mock_sandbox):
254254

255255
assert toolset._playwright_sync is None
256256

257-
def test_concurrent_get_playwright_creates_only_one_connection(
257+
def test_concurrent_get_playwright_each_thread_gets_own_connection(
258258
self, toolset, mock_sandbox
259259
):
260-
"""测试并发调用 _get_playwright 只创建一个连接,不会泄漏
260+
"""测试并发调用 _get_playwright 时每个线程各自创建连接
261261
262-
所有工作线程在同一 executor 内并发运行(即创建线程仍存活)
263-
应复用同一连接,不会触发重建
262+
Playwright Sync API 的 greenlet 绑定到创建它的 OS 线程
263+
不能跨线程共享。每个工作线程必须创建自己的连接
264264
"""
265265
start_barrier = threading.Barrier(5)
266-
# Keep all threads alive until every thread has obtained playwright,
267-
# simulating concurrent workers within the same executor context.
268-
hold_barrier = threading.Barrier(5)
269266
results: list = []
270267

271268
def worker():
272269
start_barrier.wait()
273270
p = toolset._get_playwright(mock_sandbox)
274271
results.append(p)
275-
hold_barrier.wait() # stay alive so is_alive() == True for peers
276272

277273
threads = [threading.Thread(target=worker) for _ in range(5)]
278274
for t in threads:
279275
t.start()
280276
for t in threads:
281277
t.join()
282278

279+
# Every thread must have received a connection
283280
assert len(results) == 5
284-
assert all(p is results[0] for p in results)
285-
mock_sandbox.sync_playwright.assert_called_once()
286281

287282

288283
class TestBrowserToolSetClose:
@@ -392,21 +387,22 @@ def second_call():
392387
# A new connection must have been created for the second call
393388
assert mock_sandbox.sync_playwright.call_count == 2
394389

395-
def test_get_playwright_live_thread_not_recreated(
390+
def test_get_playwright_different_live_thread_recreates_connection(
396391
self, toolset, mock_sandbox
397392
):
398-
"""测试创建线程仍存活时不重建连接(并发安全)
393+
"""测试从不同线程调用时,即使创建线程仍存活也会重建连接
399394
400-
即使在不同线程中调用,只要创建线程仍然存活,就应复用同一连接。
401-
这模拟同一 executor 内并发工具调用的场景。
395+
Playwright Sync API 的 greenlet 绑定到创建它的 OS 线程,
396+
即使创建线程仍存活,在另一个线程上调用也不安全。
397+
每个调用线程必须获得自己的连接。
402398
"""
403399
results: list = []
404400

405401
# Create connection in main thread first
406402
toolset._get_playwright(mock_sandbox)
407403
# The creating thread (main test thread) is still alive
408404

409-
# Another thread should reuse the same connection
405+
# A different thread must receive its own new connection
410406
def worker():
411407
p = toolset._get_playwright(mock_sandbox)
412408
results.append(p)
@@ -416,8 +412,8 @@ def worker():
416412
t.join()
417413

418414
assert len(results) == 1
419-
assert results[0] is toolset._playwright_sync
420-
mock_sandbox.sync_playwright.assert_called_once()
415+
# A new connection must have been created for the worker thread
416+
assert mock_sandbox.sync_playwright.call_count == 2
421417

422418
def test_reset_playwright_clears_thread(self, toolset, mock_sandbox):
423419
"""测试 _reset_playwright 清理线程引用"""
@@ -452,27 +448,61 @@ def toolset(self, mock_sandbox):
452448
ts._ensure_sandbox = MagicMock(return_value=mock_sandbox)
453449
return ts
454450

455-
def test_greenlet_thread_error_resets_playwright_and_sandbox(
451+
def test_greenlet_error_resets_playwright_keeps_sandbox_and_retries(
456452
self, toolset, mock_sandbox
457453
):
458-
"""测试 greenlet 线程绑定错误触发 Playwright 和沙箱重置
454+
"""測試 greenlet.error 触发 Playwright 重置、保留沙箱并重试
459455
460-
'cannot switch to a different thread' 错误发生时
461-
必须重置缓存的 Playwright 实例,避免后续调用持续失败
456+
greenlet.error 发生时,沙箱本身仍然健康(这是客户端线程亲和性问题)
457+
只需重置 Playwright 连接并在当前线程重试,不应销毁沙箱
462458
"""
459+
try:
460+
from greenlet import error as GreenletError
461+
except ImportError:
462+
pytest.skip("greenlet not installed")
463+
464+
call_count = 0
463465

464466
def callback(sb):
465-
raise Exception(
467+
nonlocal call_count
468+
call_count += 1
469+
if call_count == 1:
470+
raise GreenletError(
471+
"cannot switch to a different thread (which happens to have"
472+
" exited)"
473+
)
474+
return {"success": True}
475+
476+
result = toolset._run_in_sandbox(callback)
477+
478+
assert result == {"success": True}
479+
assert call_count == 2
480+
toolset._reset_playwright.assert_called_once()
481+
# Sandbox must be preserved — the error is client-side thread affinity,
482+
# not a sandbox crash.
483+
assert toolset.sandbox is mock_sandbox
484+
485+
def test_greenlet_error_returns_error_if_retry_fails(
486+
self, toolset, mock_sandbox
487+
):
488+
"""测试 greenlet.error 重试失败时返回错误字典"""
489+
try:
490+
from greenlet import error as GreenletError
491+
except ImportError:
492+
pytest.skip("greenlet not installed")
493+
494+
def callback(sb):
495+
raise GreenletError(
466496
"cannot switch to a different thread (which happens to have"
467497
" exited)"
468498
)
469499

470500
result = toolset._run_in_sandbox(callback)
471501

472502
assert "error" in result
473-
assert "cannot switch to" in result["error"]
474503
toolset._reset_playwright.assert_called_once()
475-
assert toolset.sandbox is None
504+
# Sandbox still preserved even after retry failure
505+
assert toolset.sandbox is mock_sandbox
476506

477507
def test_non_greenlet_unexpected_error_does_not_reset(
478508
self, toolset, mock_sandbox

0 commit comments

Comments
 (0)