Skip to content

[RELIABILITY] Bound adapter steps and campaign wall clock (MUT-022, MUT-045) - #96

Merged
CodewithJha merged 2 commits into
CodewithJha:mainfrom
BasilZafar11:fix/issue-70-step-timeouts
Oct 6, 2026
Merged

CodewithJha merged 2 commits into
CodewithJha:mainfrom
BasilZafar11:fix/issue-70-step-timeouts

Conversation

@BasilZafar11

@BasilZafar11 BasilZafar11 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #70

Summary

  • Check the campaign wall-clock budget between candidates while preserving violations found by a candidate that crosses the limit.
  • Add a configurable per-step timeout (default 60 seconds); timed-out calls become candidate error traces.
  • Add slow-adapter tests for timeout, budget boundaries, and violation precedence.
  • Document that a timed-out daemon worker keeps using the shared adapter because Python cannot cancel it.

Verification

  • Focused runner/campaign tests: 17 passed.
  • Unit group: 482 passed; tests/unit/test_package_release.py: 5 passed when run separately.
  • Reliability group: 2 passed.
  • Integration group did not complete in this Windows sandbox (the run stalled during startup). The combined unit run also hit sandbox restrictions in the nested uv build; the package-release file passed when run separately.

@vercel

vercel Bot commented Oct 6, 2026

Copy link
Copy Markdown

@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 CodewithJha left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 step turns into a candidate trace with status="error" and error="adapter_step_timeout: …", and the campaign keeps going. Using daemon=True is the right call: a non-daemon worker, or a ThreadPoolExecutor, 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_step and test_hung_candidate_step_is_recorded_as_candidate_error both fail. If I remove the per-candidate budget checks, test_wall_clock_budget_stops_between_candidates fails.

What still needs to change

  1. Blocker: a violation found inside the budget gets reported as budget. The post-candidate _budget_exceeded check runs before the stop_on_first_violation branch. So if a violating candidate's evaluation pushes elapsed time over wall_clock_seconds, the engine returns status="completed", reason="budget" and never emits VIOLATION_DETECTED. Reproduced: FakeRefundAdapter sleeping 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 are CANDIDATE_SCORED → CAMPAIGN_COMPLETED (no VIOLATION_DETECTED)

    Hosted/SSE consumers and anything keyed on status lose the finding's headline. Running out of budget should never hide a violation that was already found.

  2. 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 asserts reason == "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.

  3. _budget_result is 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 runs on_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]] makes result an object, so mypy reports 4 new attr-defined errors at result.assistant_message / .tool_calls / .tool_results / .raw (main is 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 missing agents module). 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 step that sleeps and returns an AdapterTurnResult with no tool calls. Then assert reason == "budget", not result.violated, and adapter.calls == len(result.candidates). Also add one small test that a violating slow candidate still returns status == "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):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BasilZafar11
BasilZafar11 force-pushed the fix/issue-70-step-timeouts branch from 43baa5e to ff7a302 Compare October 6, 2026 13:01
@BasilZafar11

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I’ve addressed all the requested changes:

  • Removed the post-candidate budget check so a discovered violation takes priority.
  • Reused _budget_result for the generation-start budget path.
  • Updated the budget test to use a non-violating adapter.
  • Added a regression test confirming that a slow violating candidate still returns status="violation".
  • Simplified the runner result/error handling and fixed its typing.
  • Added a comment documenting that timed-out daemon workers may continue using the shared adapter.
    Verification:
  • Focused runner and campaign tests: 17 passed
  • Ruff import-order check: passed
  • Mypy runner check: passed
  • Reliability tests: 2 passed
    The updated commit is ff7a302. Please re-review when you have a chance.

@CodewithJha
CodewithJha self-requested a review October 6, 2026 14:41
@CodewithJha
CodewithJha merged commit 3262c7b into CodewithJha:main Oct 6, 2026
5 of 6 checks passed
CodewithJha pushed a commit that referenced this pull request Oct 6, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
@CodewithJha

Copy link
Copy Markdown
Owner

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 ff7a302:

  • The blocker is gone. A slow candidate that violates under a tight wall_clock_seconds now reports status=violation, and the event stream includes violation.detected, the same as main.
  • The tests catch the right regressions. If I remove the worker thread, both timeout tests fail. If I remove the pre-candidate budget check, test_wall_clock_budget_stops_between_candidates fails. If I put the post-candidate check back, test_violation_wins_when_slow_candidate_exceeds_budget fails. Each guard has a test that fails without it.
  • Everything else still holds. The full suite passes (636), mypy is clean on the touched files, all CI jobs are green, the timing tests passed 25 of 25 repeated runs, and the offline mutiny run sample output is identical to main.
  • The cleanup landed. _budget_result now replaces the old inline generation-start block instead of duplicating it. The runner is simpler, and the ponytail: comment documents the known limit: a timed-out step keeps running on the shared adapter.

#70 is closed and the audit index is updated.

If you'd like another issue in the same area (core engine and reliability):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RELIABILITY] MUT-022, MUT-045: wall_clock_seconds does not interrupt in-flight steps

2 participants