refactor: Replace ProcessPoolExecutor with asyncio for evaluation

[simonrosenberg]

Summary

Replaces ProcessPoolExecutor with asyncio + semaphore-based concurrency for evaluation orchestration. Since the workload is I/O-bound (HTTP calls to LLM proxy + runtime API), this eliminates the 30x memory multiplication from spawning 30 worker processes — the root cause of OOM failures on long-running SWTBench evaluations (#441).

Key Changes

evaluation.py — Core refactor

• ProcessPoolExecutor → asyncio: New _run_iterative_mode_async() with asyncio.run() bridge from sync _run_iterative_mode()
• _run_attempt_async(): Semaphore-controlled concurrency (asyncio.Semaphore(num_workers)), tasks via asyncio.create_task(), timeout detection via asyncio.wait(FIRST_COMPLETED)
• _process_one_mp → _process_one_sync: Runs in thread executor via asyncio.to_thread() — same sync SDK operations, no code change needed in subclasses
• Thread-safety additions: asyncio.Lock for file writes, threading.Lock for LMNR_SPAN_CONTEXT env var, _safe_end_span() for cross-thread OpenTelemetry span cleanup

worker_context.py — New module

Consolidates all per-thread routing infrastructure into a single threading.local():

• _RoutedFileHandler / _RoutedConsoleHandler: Single root logger handlers that delegate to per-thread formatters/file handlers
• _ThreadLocalWriter: Replaces sys.stdout/sys.stderr with thread-aware writers that route print() to per-instance log files
• instance_context(): Single context manager replacing both setup_instance_logging() and redirect_stdout_stderr()
• initialize(): One-time setup (idempotent) — installs handlers, writers, suppresses noisy OTel context-detach errors

console_logging.py

• Removed setup_instance_logging() (moved into worker_context.instance_context())

Tests

• Added test_async_evaluation.py (7 tests + 2 integration tests): semaphore concurrency, to_thread, timeout detection, task cancellation, file locking, end-to-end evaluation run, timeout cancellation
• Updated test_keyboard_interrupt.py: Adapted for threads (no child processes to enumerate)
• Updated test_workspace_cleanup.py: _process_one_mp → _process_one_sync

Dependencies

• Added pytest-asyncio for async test support
• asyncio_mode = "auto" in pyproject.toml

Memory Impact

Architecture	Memory Usage (30 workers)
ProcessPoolExecutor	30 × (~100-400 MB) = 3-12 GB
asyncio + threads	1 × (~200-500 MB) = 200-500 MB

Backward Compatibility

• Evaluation.run() API unchanged
• num_workers still controls concurrency
• Same timeout handling, retry logic, resource factor escalation
• Subclass contract unchanged (prepare_instances, prepare_workspace, evaluate_instance)

Fixes #441

---

Tracking issue: OpenHands/evaluation#304
Umbrella issue: OpenHands/evaluation#303

[juanmichelini]

This looks interesting!

testing it with GLM-5 and swebenchmultimodal that has has some deadlocks.https://github.com/OpenHands/software-agent-sdk/actions/runs/22772705315
also testing it against the swtbench images build https://github.com/OpenHands/software-agent-sdk/actions/runs/22772664611

[juanmichelini]

Tested it with swebenchmultimodal GLM-5 (which works on main)
and it consistently gives the same error below.

Can you add links to successful runs?

Evaluation Failed

Evaluation: 22906539712-glm-5
Model: litellm_proxy/openrouter/z-ai/glm-5
Benchmark: swebenchmultimodal
Exit Code: 5

Log Tail:

  File "/workspace/benchmarks/vendor/software-agent-sdk/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py", line 1031, in _handle_conversation_status
    raise ConversationRunError(
openhands.sdk.conversation.exceptions.ConversationRunError: Conversation run failed for id=53b37d6f-d74f-48f6-93e1-2e70f68e0444: Remote conversation ended with error
2026-03-11 20:27:11,448 - ERROR - opentelemetry.context - Failed to detach context
Traceback (most recent call last):
  File "/workspace/benchmarks/.venv/lib/python3.12/site-packages/opentelemetry/context/init.py", line 155, in detach
    _RUNTIME_CONTEXT.detach(token)
  File "/workspace/benchmarks/.venv/lib/python3.12/site-packages/opentelemetry/context/contextvars_context.py", line 53, in detach
    self._current_context.reset(token)
ValueError: <Token var=<ContextVar name='current_context' default={} at 0x7ebd72de98f0> at 0x7ebd077059c0> was created in a different Context
ERROR: opentelemetry.context: Failed to detach context
2026-03-11 20:27:11,448 - ERROR - opentelemetry.context - Failed to detach context
Traceback (most recent call last):
  File "/workspace/benchmarks/.venv/lib/python3.12/site-packages/opentelemetry/context/init.py", line 155, in detach
    _RUNTIME_CONTEXT.detach(token)
  File "/workspace/benchmarks/.venv/lib/python3.12/site-packages/opentelemetry/context/contextvars_context.py", line 53, in detach
    self._current_context.reset(token)
ValueError: <Token var=<ContextVar name='current_context' default={} at 0x7ebd72de98f0> at 0x7ebd07510940> was created in a different Context
ERROR: opentelemetry.context: Failed to detach context
jq: parse error: Invalid numeric literal at line 1, column 4

[all-hands-bot]

Taste Rating: 🟡 Acceptable

The core refactor addresses a real production problem (30× memory from worker processes causing OOM). The asyncio approach is fundamentally sound. However, the solution trades process memory for code complexity in the logging infrastructure. The new worker_context.py module adds significant indirection that could be simpler.

Positive: Integration tests were added since the last review, addressing the critical testing gap.

Concern: We're routing everything through thread-local getattr() chains and swallowing exceptions silently in multiple places. This feels like fighting Python's threading model rather than working with it.

---

[all-hands-bot]

🟠 Important - Indirection Chain: This handler uses getattr(_ctx, "file_handler", None) to route to per-thread handlers. Three separate routing classes (_RoutedFileHandler, _RoutedConsoleHandler, _ThreadLocalWriter) all follow this pattern.

Question: Is thread-local routing actually necessary here? Could we instead pass explicit logger/file objects to the worker function? The current design requires understanding contextvars, threading.local(), and the routing logic to debug logging issues.

Alternative approach: Pass a LogContext object explicitly to each worker thread instead of relying on thread-local magic. Makes data flow explicit.

[simonrosenberg]

Thread-local routing is necessary here because:

1. Third-party code logs too — the SDK, OpenTelemetry, litellm, and other deps all call logging.getLogger() and get the same root logger. We can't pass explicit loggers to those libraries.
2. stdout/stderr are process-global — print() calls from SDK code, tqdm, and third-party libs write to sys.stdout. The only way to route per-thread is to replace sys.stdout with a thread-local writer.

An explicit LogContext object would work for our own code, but wouldn't capture output from the ~20+ transitive dependencies that also log/print during evaluation. The thread-local approach is the standard pattern for this (same as what Django uses for locale.activate() per-request).

That said, the consolidation into a single _ctx with one module makes the routing much easier to follow than the previous scattered implementation.

[all-hands-bot]

🔴 Critical - Silent Failure: except Exception: pass swallows ALL exceptions during file write. If the file handler is closed, disk is full, or permissions changed, logs silently disappear.

Better approach:

try:
    fh.stream.write(record_msg + "\n")
    fh.stream.flush()
except (OSError, ValueError) as e:
    # Log to stderr as fallback so we know logging failed
    sys.__stderr__.write(f"[LOGGING FAILED] {record_msg}\n")

Logging infrastructure failures should not be invisible.

[all-hands-bot]

🟡 Suggestion - Exception Handling: ValueError catch here handles "I/O operation on closed file" but the fallback writes to self._original which could also be closed.

try:
    return target.write(s)
except (ValueError, OSError):
    # If both streams fail, at least try stderr
    try:
        return self._original.write(s)
    except Exception:
        sys.__stderr__.write(s)  # Last resort

[all-hands-bot]

🟡 Suggestion - Sentinel Pattern: Using start_time: Optional[float] = None as a sentinel for "not started yet" was flagged in previous review. The code now has to check if st is not None before timeout calculation (line 587).

Cleaner approach: Don't track start_time in PendingInstance at all. Instead, store it in a separate dict only when the task actually starts:

active_start_times: dict[asyncio.Task, float] = {}

# When semaphore acquired:
active_start_times[task] = time.monotonic()

# Timeout check:
for task in pending:
    start = active_start_times.get(task)
    if start and now - start > timeout:
        timed_out.append(task)

Eliminates the None check entirely ("good taste" - no special case).

[all-hands-bot]

🟠 Important - Silent Exception: Bare except Exception: pass with only a comment explaining why.

Issue: If span.end() fails for a DIFFERENT reason (network error, SDK bug), we'll never know. The comment says "contextvars tokens created in main thread cannot be detached from worker thread" but we're catching ALL exceptions.

try:
    eval_span.end()
except RuntimeError as e:
    # Expected: contextvars token detach from worker thread
    if "context" not in str(e).lower():
        logger.warning(f"Unexpected span.end() error: {e}")
except Exception as e:
    logger.warning(f"Unexpected span.end() error: {e}")

Catch the specific error we expect, log unexpected ones.

[all-hands-bot]

🟢 Acceptable - Integration Test Added: This test creates a real Evaluation instance, runs multiple instances through the async path, verifies error handling, and checks output files.

This addresses the critical "Missing Integration Test" feedback from the previous review. The test exercises the actual evaluation flow, not just asyncio primitives.

Also good: test_evaluation_timeout_cancels_instance verifies timeout behavior end-to-end.

[all-hands-bot]

🟡 Suggestion - Design Smell: The comment "Reset start_time to NOW so timeout counts from when instance actually begins running" reveals we're working around the data structure instead of with it.

This is the continuation of the sentinel pattern issue. We're storing start_time in PendingInstance before the instance starts (set to None), then updating it later.

Root cause: PendingInstance is created before the task starts but needs different state after it starts. Consider splitting into two types:

@dataclass
class QueuedInstance:
    instance: EvalInstance
    task: asyncio.Task
    datapoint_id: UUID | None

@dataclass  
class RunningInstance(QueuedInstance):
    start_time: float  # Always set, never None

Move from queued → running when semaphore acquired. No None checks needed.

[all-hands-bot]

Taste Rating: 🔴 Blocking Issues + Over-Engineering

The core refactor (ProcessPoolExecutor → asyncio) solves a real production problem (30× memory multiplication causing OOM). The approach is theoretically sound for I/O-bound workloads.

However:

---

🔴 BLOCKING: Runtime Failures Reported

@juanmichelini reported consistent failures with GLM-5 that works on main:

"Tested it with swebenchmultimodal GLM-5 (which works on main) and it consistently gives the same error"

This must be debugged and fixed before merge. The PR description claims successful evaluation but provides no evidence (no links to successful runs, no logs proving GLM-5 works).

Required: Add an Evidence section to the PR description with:

• Links to successful evaluation runs (especially GLM-5)
• Complete logs showing the async refactor works end-to-end
• Comparison with main branch results to prove no regression

---

🔴 Critical: Silent Exception Handling Masks Bugs

benchmarks/utils/worker_context.py:63 - _RoutedFileHandler.emit():

except (OSError, ValueError):
    # fall back to stderr so the message isn't silently lost.
    if sys.__stderr__:
        try:
            sys.__stderr__.write(record_msg + "\n")
        except Exception:
            pass

Problem: If file writes fail (closed file, disk full), you fall back to stderr, then if THAT fails, you silently pass. The actual error is lost. This could be hiding the root cause of the GLM-5 failures.

Fix: Don't silently swallow exceptions. At minimum, log the failure and raise so we know something is broken.

benchmarks/utils/evaluation.py:762-770 and :899-910 - Span cleanup:

except RuntimeError:
    # Expected: contextvars tokens created in the main
    # thread cannot be detached from a worker thread.
    pass
except Exception as e:
    logger.warning("[worker] Unexpected span.end() error: %s", e)

Problem: You're catching ALL RuntimeErrors assuming they're the "expected" contextvars issue. What if a different RuntimeError occurs that indicates a real bug?

Fix: Check the error message before ignoring:

except RuntimeError as e:
    if "cannot be detached" not in str(e):
        logger.error(f"[worker] Unexpected RuntimeError: {e}")
        raise
    logger.debug(f"[worker] Ignored expected contextvars error")

---

🟡 Architecture: Over-Engineered Logging Infrastructure

benchmarks/utils/worker_context.py - 330 lines of thread-routing machinery:

• _RoutedFileHandler with getattr(_ctx, "file_handler", None) routing
• _RoutedConsoleHandler with getattr(_ctx, "console_formatter", None) routing
• _ThreadLocalWriter with getattr(_ctx, "log_file", None) routing
• Three layers of fallback exception handling
• Global initialization guard with locking

This is too much machinery for "each thread needs its own log file."

Fundamental question: Why do we need thread-local routing at all? Each thread could just open its own log file directly and configure its own logger. No routing. No threading.local(). No getattr() magic.

If third-party code (SDK, OpenTelemetry) MUST use the root logger and you can't pass explicit loggers, then document WHY this complexity is unavoidable. Right now it looks like we're solving a problem we created.

"If you need more than 3 levels of indirection, you're doing it wrong." - This has multiple levels of getattr-based routing with fallback chains. That's a code smell.

---

🟠 Important: Fighting the Framework

The repeated except RuntimeError for "contextvars tokens cannot be detached from worker threads" indicates we're fighting Python's design.

This is a warning sign. If OpenTelemetry spans + contextvars don't work naturally with threads, maybe we're using the wrong concurrency model.

Alternatives to consider:

1. Does OpenTelemetry have a thread-safe span API?
2. Could we use ProcessPoolExecutor with worker recycling (solves memory issue without fighting contextvars)?
3. Is the benefit of asyncio worth these workarounds?

---

🟡 Testing: Coverage Weakened

Good: test_async_evaluation.py includes integration tests that exercise real Evaluation instances.

Problem: Tests use mocks for workspace/LaminarService. They prove asyncio flow works, but not that it integrates correctly with the real SDK.

Given the reported GLM-5 failures, the mocked tests clearly aren't catching integration issues.

test_keyboard_interrupt.py: The old test verified worker processes were cleaned up. The new test just checks "process exits." Lost coverage: we don't verify cleanup behavior or that threads actually started.

---

Verdict: ❌ Needs Rework

Cannot merge until:

1. ✅ Debug and fix the GLM-5 failures reported by @juanmichelini
2. ✅ Add evidence section with successful run links
3. ✅ Fix silent exception handling (at minimum, check error messages before ignoring)
4. 💭 Consider simplifying the logging infrastructure (current approach is over-engineered)

Key Insight: You're trading 30× memory for code complexity and fighting Python's concurrency primitives. The core idea is sound, but the implementation has too many workarounds and hidden failure modes.

[all-hands-bot]

Taste Rating: 🟡 Acceptable with Critical Issues

The core refactor (ProcessPoolExecutor → asyncio) solves a real production problem (30× memory multiplication). However, there are blocking issues that must be addressed:

1. Silent exception swallowing that could hide bugs
2. No runtime evidence despite reported failures
3. Logging infrastructure complexity that violates simplicity principles

[all-hands-bot]

🔴 Critical - Silent Exception Swallowing: This catches RuntimeError and only ignores the specific "context" error, but the fallback except Exception is too broad.

Issue: If span.end() fails for ANY other reason (network error, SDK bug, file system issue), you'll never know. The comment explains ONE failure mode, but the code silently swallows ALL exceptions.

Fix: Log unexpected exceptions at minimum:

Suggested change

	logger.warning("[worker] span.end() RuntimeError: %s", e)
	try:
	eval_span.end()
	except RuntimeError as e:
	# Contextvars tokens from main thread can't detach in worker.
	if "context" not in str(e).lower():
	logger.error("[worker] span.end() RuntimeError: %s", e, exc_info=True)
	except Exception as e:
	logger.error("[worker] span.end() unexpected error: %s", e, exc_info=True)

This is not over-engineering - it's basic observability. Silent failures make debugging impossible.

[all-hands-bot]

🔴 Critical - Same Silent Swallowing Pattern: Duplicate of the issue above. If exec_span.end() fails for a DIFFERENT reason than the contextvars issue, you won't know.

Suggested change

	try:
	exec_span.end()
	except RuntimeError as e:
	if "context" not in str(e).lower():
	logger.error("[worker] exec_span.end() RuntimeError: %s", e, exc_info=True)
	except Exception as e:
	logger.error("[worker] exec_span.end() unexpected error: %s", e, exc_info=True)

[all-hands-bot]

🟠 Important - Fallback Still Fails: The except block handles (OSError, ValueError) but writes to sys.__stderr__ which could ALSO be None or closed.

Better approach - fail fast if logging is broken:

Suggested change

	if sys.__stderr__:
	try:
	fh.stream.write(msg + "\n")
	fh.stream.flush()
	except Exception as e:
	# Last-resort: try stderr, but don't swallow if both fail
	if sys.__stderr__:
	try:
	sys.__stderr__.write(f"[logging failed: {e}] {msg}\n")
	except Exception:
	pass # Nothing left to do

If logging is completely broken, we want to know, not silently lose logs.

[all-hands-bot]

🟡 Suggestion - Over-Engineered Forwarding: You've implemented explicit properties for encoding, closed, isatty(), and fileno() to "avoid hiding bugs" per the comment on line 98.

Question: Which code actually calls these? If nothing uses isatty() or fileno() on the wrapped streams, this is premature.

Simpler approach - Only implement what's actually needed. If tqdm or SDK code breaks, THEN add the specific property. This is 20 lines that might not solve any real problem.

[all-hands-bot]

🟡 Suggestion - Complex Setup/Teardown: The instance_context() function does:

1. Initialize logging (if needed)
2. Open two files
3. Set 4+ thread-local attributes
4. Print startup message with rich/plain branching
5. Temporarily change console_level for startup
6. Clean up all thread-local attrs
7. Close files

This is a lot of moving parts. The comment on line 7 says "thread-local routing is necessary" because third-party code logs to root logger. That's true, but the implementation has 4 routing classes + thread-local state + dual-mode formatters.

Not blocking, but this is the kind of complexity that creates bugs. If a file fails to close, does thread-local cleanup still happen? (Answer: yes, but only because finally block clears attrs - easy to miss)

[all-hands-bot]

🟢 Acceptable - Integration Test Added: This test creates a real Evaluation instance, runs 4 instances (including one that fails), and verifies error outputs are created.

This addresses the "Missing Integration Test" feedback from previous reviews. The test exercises the async path end-to-end.

Minor note: The test mocks LaminarService, but that's acceptable here since you're testing the evaluation orchestration, not Laminar integration.

[all-hands-bot]

🟠 Important - Test Doesn't Verify What Comment Claims: The comment says "verifies that worker threads are running before the interrupt" (line 109), but the implementation just checks process.poll() is None (line 182).

Issue: This proves the process hasn't exited, NOT that worker threads are actually running. With asyncio, the workers are threads inside the process, so you can't enumerate them from outside.

The test is still valid (proves clean shutdown), but the comment overpromises. Change line 109-110 to:

Suggested change

	"""Test that worker threads are properly cleaned up on KeyboardInterrupt.
	The asyncio evaluation uses threads (via asyncio.to_thread()), not child
	processes. This test verifies that the process exits cleanly after SIGINT.
	Worker thread cleanup is implicit - if threads were stuck, the process
	wouldn't exit within the timeout.
	"""

[all-hands-bot]

Taste Rating: 🟡 Acceptable

The core refactor (ProcessPoolExecutor → asyncio) solves a real production problem (30× memory from worker processes) and is architecturally sound for I/O-bound workloads. The simplified logging approach is pragmatic.

However: The exception handling around span lifecycle is brittle, and there's unnecessary state splitting in the timeout tracking. These should be cleaned up for maintainability.

[all-hands-bot]

🟠 Important - Fragile Exception Handling: Catching Exception and checking if "context" is in the string is brittle. If the error message format changes or there's a different contextvars error, this breaks.

Better approach - be explicit about what we're ignoring:

try:
    eval_span.end()
except LookupError as e:
    # Contextvars tokens from main thread can't detach in worker thread
    # This is expected due to Python's contextvars design
    pass
except Exception as e:
    logger.error("[worker] Unexpected span.end() error: %s", e, exc_info=True)

(Contextvars raises LookupError for token issues, not RuntimeError with "context" in the message)

[all-hands-bot]

🟠 Important - Duplicate Fragile Pattern: Same brittle exception handling as above. If you must keep string matching, at least extract it to a helper function so the workaround is in one place:

def safe_end_span(span, context_name="worker"):
    """End span, ignoring contextvars threading limitations."""
    try:
        span.end()
    except LookupError:
        pass  # Expected: contextvars tokens can't cross thread boundaries
    except Exception as e:
        logger.warning(f"[{context_name}] span.end() error: %s", e)

[all-hands-bot]

🟡 Suggestion - Split State: You track task start times in two places:

1. active_start_times dict (line 467)
2. PendingInstance.task (line 458)

This creates two sources of truth. The comment "Only tasks present in this dict are subject to timeout" reveals this is a workaround.

Cleaner approach: Add start_time field to PendingInstance and set it when the task acquires the semaphore. Check pending_instances[task].start_time directly instead of a separate dict.

[all-hands-bot]

🟡 Suggestion - Comment Mismatch: The comment on line 174 says "Wait for worker threads to start" and explains why we can't enumerate them, but then says "Instead we verify the process is still alive — if workers hadn't started their 60 s sleep, the process would have exited already."

Issue: This doesn't actually prove threads started — the main asyncio loop keeps the process alive even if no threads are running yet. The 3-second sleep is a heuristic.

Be honest in the comment:

# Wait for worker threads to start (heuristic - we can't enumerate threads from outside)
time.sleep(3)

[all-hands-bot]

🟢 Acceptable - Pragmatic Solution: This shared-log-then-split approach is simpler than the previous complex thread-local routing. While pure structured logging (with instance_id as a field) might be even simpler, this is a reasonable pragmatic solution given the requirement to split logs into per-instance files.

The post-processing in split_logs() is straightforward and easier to reason about than the previous routing classes.

[all-hands-bot]

🟢 Good - Integration Test: This test creates a real Evaluation instance and runs multiple instances through the async path, including error handling. This addresses the previous review's concern about missing integration tests.

The mock workspace is acceptable here — testing the full Docker runtime would be too slow for unit tests.

[all-hands-bot]

Taste Rating: 🔴 Blocking Issues + Unjustified Complexity

The core refactor (ProcessPoolExecutor → asyncio) solves a real problem (30× memory multiplication causing OOM). However:

🔴 BLOCKING: No Evidence This Works

@juanmichelini reported consistent failures with GLM-5 that works on main. The PR description claims memory benefits but provides no links to successful evaluation runs. Without proof this actually works in production, we're trading known-working code for unproven complexity.

🔴 CRITICAL: Complexity Explosion

We've traded 30× memory for 331 lines of thread-routing infrastructure that:

• Shouldn't need to exist (use structured logging with instance_id field)
• Has 3-level exception fallback chains masking real failures
• Uses thread-local state + global initialization + routing classes

This violates "good taste" - we're handling edge cases instead of eliminating them.

Required Before Merge:

1. Links to successful SWTBench/multimodal evaluation runs showing this works
2. Simplify worker_context.py or justify why 331 lines of routing is necessary
3. Address the specific inline comments below

---

This review was created by an AI assistant (OpenHands) on behalf of the repository maintainers.

[all-hands-bot]

🔴 Critical - Over-Engineering: This entire 331-line module exists to route logs/stdout to per-thread files. Why?

The Real Problem: We're solving logging the hard way because we're stuck with print() and global logging.getLogger() from third-party code.

Simpler Approach: Use structured logging with instance_id as a field, then filter/split at write time:

logger.info("Processing", extra={"instance_id": inst.id})

Then a single log handler can write to per-instance files by checking record.instance_id.

This eliminates:

• Thread-local state tracking
• All the routing classes
• Complex initialization guards
• 200+ lines of indirection

If you MUST keep this approach, justify why structured logging won't work.

[all-hands-bot]

🟠 Important - Fragile Fallback Chain: Three levels of exception handling for a simple write:

1. Try target.write()
2. Catch, try self._original.write()
3. Catch, try sys.stderr.write()

Issue: This masks real problems. If the target fails (disk full, permissions), we should KNOW about it, not silently fall back.

Better Approach: Fail fast

def write(self, s: str) -> int:
    target = self._target()
    try:
        return target.write(s)  # type: ignore[union-attr]
    except (ValueError, OSError) as e:
        # File closed/broken is a real error, not something to hide
        logger.error(f"Log write failed: {e}")
        raise

If writes are failing, we need to know, not paper over it.

[all-hands-bot]

🟡 Acceptable - Pragmatic Workaround: Extracting _safe_end_span is good, but the generic Exception catch is still concerning.

Current Code:

except LookupError:  # Expected
    pass
except Exception as e:
    logger.warning(...)  # Could hide real bugs

Suggestion: Be more specific about what we're ignoring:

except LookupError:
    # Expected: span created in main thread, ended in worker
    pass
except RuntimeError as e:
    if "context" in str(e).lower():
        pass  # Another contextvars variation
    else:
        raise  # Real SDK error

This way we don't accidentally swallow legitimate span.end() failures.

[all-hands-bot]

🟡 Suggestion - Sentinel Pattern: Using start_time: float | None = None as sentinel creates split state (also tracked in dict at line 435).

Issue: Two sources of truth for when instance started. The comment at line 474 reveals this: "Reset start_time to NOW so timeout counts from when instance actually begins running".

Cleaner Design: Don't track start_time in PendingInstance at all. Only track in the pending_instances dict when the semaphore is acquired:

@dataclass
class PendingInstance:
    instance: EvalInstance
    datapoint_id: UUID | None = None
    task: asyncio.Task | None = None
    # No start_time here

# In _run_attempt_async:
active_start_times: dict[asyncio.Task, float] = {}  # Only source of truth

async with semaphore:
    task = asyncio.current_task()
    active_start_times[task] = time.monotonic()  # Set once, when work starts

This eliminates the None checks and "reset" logic.

[all-hands-bot]

🟠 Important - Silent Failure: The fallback to sys.__stderr__ could also fail, and we'd never know.

except (OSError, ValueError):
    if sys.__stderr__:
        try:
            sys.__stderr__.write(record_msg + "\n")
        except Exception:
            pass  # Silently swallowed

Better:

except (OSError, ValueError) as e:
    # Last resort: write to stderr if available
    if sys.__stderr__:
        try:
            sys.__stderr__.write(f"LOG WRITE FAILED: {e}\n{record_msg}\n")
        except Exception:
            pass  # Truly nothing left to do
    # Don't return silently - at least raise so we know logging is broken
    raise RuntimeError(f"All log write attempts failed: {e}")

If we can't write logs anywhere, the evaluation should probably fail rather than continue silently.

[all-hands-bot]

🟡 Suggestion - Mock Workspace: This integration test uses workspace = Mock() with __exit__ = Mock(), which doesn't exercise the actual workspace cleanup path.

Current Coverage: Tests the asyncio orchestration but not the SDK integration.

Suggestion: Consider adding one test with a real workspace (even if it's just a local Docker container) to verify:

1. Thread executor actually runs SDK operations
2. Workspace cleanup happens correctly
3. Conversation archives are captured

The mock is acceptable for testing the async orchestration patterns, but we should have at least one end-to-end test that proves the SDK integration works.

This partially addresses previous review feedback about integration tests, but real workspace testing would be more convincing.

[all-hands-bot]

🟡 Suggestion - Weak Verification: Comment says "Heuristic wait for worker threads to start" with 3-second sleep + "rough guess".

Issue: This doesn't actually verify threads started, just that the process didn't exit.

Suggestion: Add verification that proves work started:

# Check that at least one log file was created
log_dir = tmp_path / "logs"
time.sleep(3)  # Wait for startup
log_files = list(log_dir.glob("instance_*.log"))
assert len(log_files) > 0, "No worker threads started (no log files created)"

This provides concrete evidence that worker threads actually began processing before the interrupt.

[simonrosenberg]

Validation Runs (2026-03-19)

All runs today using benchmarks_branch=openhands/asyncio-evaluation-refactor:

Successful Runs

Benchmark	Model	Run ID	Eval Limit	Result	Duration	Notes
swebench	claude-sonnet-4-5	23293907394	10	✅ Success	~99 min	1st run, 1 transient retry
gaia	claude-sonnet-4-5	23293908685	10	✅ Success (7/10 resolved)	~23 min	1st run, 1 transient retry
swebench	claude-sonnet-4-5	23298944024	10	✅ Success (10/10 resolved)	~57 min	Retrigger after branch update
gaia	claude-sonnet-4-5	23298945423	10	✅ Success (6/10 resolved)	~14 min	Retrigger after branch update
swtbench	qwen3-coder-next	23276037687	-	✅ Success	-	Early morning run
swtbench	qwen3-coder-next	23299344369	-	✅ Success	-	Validation run 2, 0 retries
swtbench	qwen3-coder-next	23299573896	-	✅ Success	-	Validation run 2, 0 retries

Failed / Cancelled

Benchmark	Model	Run ID	Result	Notes
swtbench	qwen-3-coder	23275993262	❌ Failed	Early morning, before branch fixes
swtbench	qwen3-coder-next	23299318283	⏹ Cancelled	Duplicate trigger

Currently Running

Benchmark	Model	K8s Job	Progress	Notes
swtbench	qwen3-coder-next	eval-23299605018-qwen3-code	14/433 (3%)	Full SWTBench run, asyncio branch at ce086e7

Summary

• 7 successful runs across swebench, gaia, and swtbench
• 3 benchmarks validated: swebench, gaia, swtbench
• 2 models validated: claude-sonnet-4-5-20250929, qwen3-coder-next
• All runs confirmed using openhands/asyncio-evaluation-refactor branch
• No OOM issues observed (the core problem this PR fixes)
• Full 433-instance SWTBench run currently in progress — this is the workload that previously OOM'd with ProcessPoolExecutor

[all-hands-bot]

Taste Rating: 🔴 Blocking Issue — No Evidence This Works

The core refactor (ProcessPoolExecutor → asyncio) solves a real production problem (30× memory multiplication from worker processes). The approach is theoretically sound for I/O-bound workloads.

However:

---

🔴 BLOCKING: Runtime Failures Reported, Zero Evidence Provided

@juanmichelini reported failures with GLM-5 that works on main:

"Tested it with swebenchmultimodal GLM-5 (which works on main) and it consistently gives the same error"

The PR description claims memory benefits but provides no links to successful evaluation runs. This is unacceptable. You're replacing working code with unproven code.

Required before merge:

1. Add an Evidence section to the PR description
2. Link to successful SWE-Bench Multi-modal runs (full eval, not toy examples)
3. Debug and resolve the GLM-5 failures @juanmichelini reported
4. Show this actually reduces memory in production (before/after metrics)

---

What Improved Since Previous Reviews

✅ Integration tests added (test_async_evaluation.py lines 207, 275) — good response to feedback
✅ Thread-safety locks for file writes (asyncio.Lock) and env vars (threading.Lock) — correct approach
✅ _safe_end_span() extracted to reduce duplication

---

Remaining Fundamental Issues

These have been discussed in previous reviews but remain unresolved:

1. worker_context.py is solving logging the wrong way (331 lines of thread-local routing)
2. Silent exception swallowing could hide real bugs
3. Split state in timeout tracking (sentinel pattern with start_time: float | None)

See inline comments for specific examples.

---

VERDICT

❌ Cannot merge: No evidence this works, user reported failures, unresolved design complexity

KEY INSIGHT: Solving a real memory problem by trading working processes for unproven threads + 331 lines of logging infrastructure is not a win until you prove it actually works.

[all-hands-bot]

🔴 Critical — Over-Engineering: This entire 331-line module exists to route logs/stdout to per-thread files because we're stuck with print() and global logging.getLogger() from third-party code.

The Real Problem: We're solving logging the hard way. Three separate routing classes (_RoutedFileHandler, _RoutedConsoleHandler, _ThreadLocalWriter) all using threading.local() indirection.

Simpler Approach: Use structured logging with instance_id as a field, then filter at write time:

logger.info("Processing", extra={"instance_id": instance.id})
# Filter in handler to write different instance_ids to different files

This would be ~50 lines instead of 331.

But: I acknowledge this has been extensively discussed in previous reviews. If you're committed to this approach, at least prove the whole refactor works first (see blocking evidence issue).

[all-hands-bot]

🟠 Important — Brittle Exception Handling: Extracting _safe_end_span reduces duplication (good), but the pattern is still concerning:

except LookupError:
    pass  # Expected
except Exception as e:
    logger.warning(...)  # Could hide real bugs

Issue: If span.end() fails for a DIFFERENT reason than the contextvars issue (network error, SDK bug, disk full), you log a warning and move on. The span data might be lost, but you'll never know why.

Better approach: Be explicit about what you're ignoring:

except (LookupError, RuntimeError) as e:
    if "context" not in str(e).lower():
        raise  # Unexpected error, don't hide it
    # else: expected contextvars thread issue

This is still hacky, but at least it won't silently swallow unrelated failures.

[all-hands-bot]

🟠 Important — Silent Failure Chain: Three levels of exception handling for a simple write:

1. Try fh.stream.write()
2. Catch → try sys.__stderr__.write()
3. Catch → pass (silently swallowed)

Issue: If both the file handler AND stderr fail (disk full, permissions, closed streams), logs disappear with no indication anything went wrong.

Better approach: Log to a last-resort location or fail fast:

try:
    fh.stream.write(msg + "\n")
    fh.stream.flush()
except (OSError, ValueError) as e:
    # Last resort: write to a dedicated error file
    with open("/tmp/evaluation_logging_errors.log", "a") as err:
        err.write(f"Failed to write log: {e}\n{msg}\n")

At least then you have a breadcrumb trail when things break.

[all-hands-bot]

🟢 Good — Integration Test Added: This test creates a real Evaluation instance, runs 4 instances (including one that fails), and verifies outputs are created.

This directly addresses previous review feedback about missing integration tests. The test exercises the async path end-to-end, which is what was needed.

However: The reported GLM-5 failures suggest there's a real-world failure mode these tests don't catch. After you get evidence of successful runs, consider adding a test case that uses the same configuration that failed for @juanmichelini.

[all-hands-bot]

🟢 Good — Thread-Safe Environment Variable Access: Using threading.Lock to protect os.environ["LMNR_SPAN_CONTEXT"] read/write is correct.

This prevents race conditions where one thread overwrites another's span context before prepare_workspace() reads it. Simple, effective, no over-engineering.

[juanmichelini]

Testing the GLM-5 and swebenchmultimodal issue https://openhands-eval-monitor.vercel.app/?run=swebenchmultimodal%2Flitellm_proxy-openrouter-z-ai-glm-5%2F23316047170%2F