Repository navigation
[RELIABILITY] Bound adapter steps and campaign wall clock (MUT-022, MUT-045) - #96
Conversation
|
@BasilZafar11 is attempting to deploy a commit to the priyanshu's projects Team on Vercel. A member of the Team first needs to authorize it. |
CodewithJha
left a comment
There was a problem hiding this comment.
Thanks @BasilZafar11, and welcome to Mutiny. This is a solid first pass at #70 (MUT-022 + MUT-045): the scope is right (core runner + engine only), it needs no live OpenAI, and the tests use slow fake adapters as the issue asked. I checked it out and ran it: 635 passed across unit/integration/reliability, CI is green (I approved the fork run), and the offline sample (examples/openai_support_agent, mutiny init && mutiny run) behaves the same as on main: status=violation, regression file written, exit 0. So moving adapter.step onto a worker thread works with the real OpenAI Agents adapter.
What is working
- Per-step timeout (MUT-045) ✓. A stuck
stepturns into a candidate trace withstatus="error"anderror="adapter_step_timeout: …", and the campaign keeps going. Usingdaemon=Trueis the right call: a non-daemon worker, or aThreadPoolExecutor, would keep a truly hung step blocking interpreter exit. - Default of 60s ✓. That is generous next to the offline sample, so the sample timing does not get flaky.
- Mutation check ✓. If I replace the thread with a direct
run_step()call,test_execute_conversation_times_out_slow_stepandtest_hung_candidate_step_is_recorded_as_candidate_errorboth fail. If I remove the per-candidate budget checks,test_wall_clock_budget_stops_between_candidatesfails.
What still needs to change
-
Blocker: a violation found inside the budget gets reported as
budget. The post-candidate_budget_exceededcheck runs before thestop_on_first_violationbranch. So if a violating candidate's evaluation pushes elapsed time overwall_clock_seconds, the engine returnsstatus="completed", reason="budget"and never emitsVIOLATION_DETECTED. Reproduced:FakeRefundAdaptersleeping 0.05s per step,wall_clock_seconds=0.01:main:status=violation reason=violation- this PR:
status=completed reason=budget violated=True, and the last events areCANDIDATE_SCORED → CAMPAIGN_COMPLETED(noVIOLATION_DETECTED)
Hosted/SSE consumers and anything keyed on
statuslose the finding's headline. Running out of budget should never hide a violation that was already found. -
The new wall-clock test locks that regression in. In
test_wall_clock_budget_stops_between_candidates, the single candidate that runs violates ([c.violated for c in result.candidates] == [True]), yet the test assertsreason == "budget". If you delete only the post-candidate check (keeping the pre-candidate one), this test fails even though the engine is now correct. The budget test needs a scenario that does not violate. -
_budget_resultis a second copy of the existing generation-start budget block (engine.py L94–111), which is still inline. Please have that call site use the helper too, so there is one budget-exit path.
Important
- Known ceiling: timed-out steps keep running. Python can't kill a thread, so the abandoned worker keeps calling into the shared adapter after the timeout. With the OpenAI Agents adapter,
reset()for the next candidate runson_reset(shared sample state) while the old step may still be executing tools. That's acceptable for this issue ("optionally cancel in-flight calls"), but it should be stated where the next reader will see it. One short# ponytail:/limitation comment by the thread start is enough. No extra machinery needed. - Typing:
queue.Queue[tuple[bool, object]]makesresultanobject, somypyreports 4 newattr-definederrors atresult.assistant_message/.tool_calls/.tool_results/.raw(mainis clean). Ruff also flags the import order (I001). Neither is in CI yet, but please don't add new findings. - PR description: the full suite does run with
uv sync --extra dev(that's how CI installs, and it avoids the missingagentsmodule). Please update the Verification section once you've rerun it.
Suggested direction (minimal diff)
- Engine: keep one check before each candidate (
if self._budget_exceeded(started): return self._budget_result(...)) and drop the post-candidate check. The pre-check on the next iteration already gives you "stop at the next candidate boundary". The generation-start check still covers the last candidate of a generation, and a violation always wins because it returns first. - Test: make the slow adapter non-violating, e.g. a
stepthat sleeps and returns anAdapterTurnResultwith no tool calls. Then assertreason == "budget",not result.violated, andadapter.calls == len(result.candidates). Also add one small test that a violating slow candidate still returnsstatus == "violation"under a tight budget. That's the regression above. - Runner: see the inline comment. The error re-raise can shrink to a single
raise.
The structure is sound. These are focused fixes, and I'm happy to re-review as soon as you push.
|
|
||
| # A single step is bounded by step_timeout_seconds. Stop at | ||
| # the next candidate boundary when the overall budget expires. | ||
| if self._budget_exceeded(started): |
There was a problem hiding this comment.
This check runs before the candidate.violated and cfg.stop_on_first_violation branch below. So a violating candidate that crosses the budget during its own evaluation is returned as status="completed", reason="budget", with no VIOLATION_DETECTED event (main returns status="violation" for the same run). The pre-candidate check at L119 already stops at the next candidate boundary, so this block can simply be removed.
| return False | ||
| return (time.monotonic() - started) >= limit | ||
|
|
||
| def _budget_result( |
There was a problem hiding this comment.
Nice extraction. Please also use it at the generation-start check (L94–111), which still builds the same CampaignResult inline. Otherwise there are two copies of the budget exit that can drift.
| trace.error = f"adapter_step_timeout: exceeded {step_timeout_seconds:g}s" | ||
| return trace | ||
| if not succeeded: | ||
| if isinstance(result, ToolsNotObservableError): |
There was a problem hiding this comment.
run_step always stores an exception on failure, so the ToolsNotObservableError branch is a duplicate of the BaseException one, and raise RuntimeError("adapter step failed") can't be reached. Typing the payload as object is also what produces the 4 new mypy attr-defined errors further down. A smaller shape: store the result/exception, t.join(step_timeout_seconds), if t.is_alive(): <timeout trace>, then raise the stored exception if there is one, otherwise use the result as an AdapterTurnResult. Please keep daemon=True as it is.
| except BaseException as exc: # propagate adapter exceptions | ||
| result_queue.put((False, exc)) | ||
|
|
||
| threading.Thread(target=run_step, daemon=True).start() |
There was a problem hiding this comment.
Please add a one-line note here about the ceiling: on timeout, the worker thread keeps running against the shared adapter (Python can't cancel it). For example: # ponytail: abandoned step keeps running on the adapter; cancel via SDK timeouts if a target supports it.
| ), | ||
| ) | ||
| result = engine.run() | ||
| assert result.reason == "budget" |
There was a problem hiding this comment.
The only candidate that runs here violates (FakeRefundAdapter + refund seeds → [True]), so this assertion encodes the regression flagged in engine.py. Please use a slow adapter that never issues issue_refund, so reason == "budget" is the correct outcome. Also add assert not result.violated.
43baa5e to
ff7a302
Compare
|
Thanks for the detailed review. I’ve addressed all the requested changes:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Merged in 3262c7b. Thank you @BasilZafar11, and welcome to Mutiny! This was a strong first contribution, and the turnaround on the review was quick and complete. What I verified on
#70 is closed and the audit index is updated. If you'd like another issue in the same area (core engine and reliability):
|
Fixes #70
Summary
Verification
tests/unit/test_package_release.py: 5 passed when run separately.uv build; the package-release file passed when run separately.