test: make parent-cancellation tool tests deterministic - #4166
Conversation
Both parent-cancellation tests decided pass/fail from wall-clock deadlines that are smaller than the runtime's own bounds, so a loaded machine could fail them even though the runtime behaved correctly. `test_parent_cancellation_does_not_wait_for_tool_cleanup` asserted the parent settles within 100ms, but `_FUNCTION_TOOL_CANCELLED_DRAIN_SECONDS` alone allows the runtime 250ms, so the deadline was below the worst case it was supposed to tolerate. Establish the ordering with events instead: wait until the tool has parked inside its cleanup, then assert the parent surfaces cancellation while that cleanup is still blocked. The remaining timeouts only detect a hang. `test_parent_cancellation_does_not_report_tool_failure_as_background_error` ended with two bare `asyncio.sleep(0)` calls and hoped the tool task and its done callback had both run. The report is delivered from a done callback, so wait for the tool to actually fail and then drain the loop's callback queue; otherwise the negative assertion could hold simply because nothing had run yet. Stress run of both tests, 40 iterations each under 32 competing CPU-bound processes: 2 failures before, 0 after.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e5a50f209
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| # The tool has to actually fail while the parent is cancelling, otherwise the | ||
| # assertion below would hold without ever reaching the reporting path. | ||
| await asyncio.wait_for(tool_failed.wait(), timeout=_CANCELLATION_HANG_GUARD_SECONDS) |
There was a problem hiding this comment.
This wait only proves the tool raised at some point; with this ordering, the asyncio.sleep(0) continuation can already be queued when execution_task.cancel() is called, so the tool can fail before parent cancellation reaches/detaches the invoke task. In that case _await_invoke_task consumes the exception from an already-completed invoke task, the background done callback path being asserted below is never exercised, and a regression that reports detached Exceptions can still pass; gate the ValueError on an event reached after parent cancellation has detached the tool, such as raising from cancellation cleanup.
Useful? React with 👍 / 👎.
| with pytest.raises(asyncio.CancelledError): | ||
| await asyncio.wait_for(execution_task, timeout=0.1) | ||
| await asyncio.wait_for(execution_task, timeout=_CANCELLATION_HANG_GUARD_SECONDS) |
There was a problem hiding this comment.
With this 5-second hang guard as the only check on when the parent surfaces cancellation, a regression that waits for a bounded cleanup drain before re-raising parent cancellation still passes whenever it finishes before the guard, even though the tool remains blocked on allow_cleanup_exit. For the does_not_wait_for_tool_cleanup behavior, make the test fail on any runtime wait-for-cleanup path independently of wall-clock speed, for example by instrumenting/monkeypatching the drain path or asserting a state transition that only happens before awaiting cleanup.
Useful? React with 👍 / 👎.
|
@LeSingh1 Can you resolve the requested changes before sending further PRs? Thanks for your understanding! |
|
Understood, and apologies for the volume — I've stopped opening new PRs here and will work through the outstanding review feedback before sending anything else. Working the Codex findings now. Two of them are correct and worth stating plainly: On #4165, the On this PR, both P2 comments land. The hang guard being the only check does mean a bounded cleanup drain that finishes under it would still pass, and the failure ordering doesn't guarantee the background done-callback path is the one being exercised. I'll pin both to state transitions rather than to timing. I'll push fixes to the existing PRs and won't open anything new until you've had a chance to look. |
…ng guard Both tests still let a regression through, because the hang guard was carrying the assertion in one and the ordering was incidental in the other. `test_parent_cancellation_does_not_wait_for_tool_cleanup` only proved the parent settled inside five seconds. A runtime that drained the cancelled tool tasks before re-raising still passed whenever that drain was bounded, even though the tool stayed blocked in its cleanup the whole time. No purely causal assertion can separate "did not wait" from "waited briefly" -- the only difference is elapsed time -- so instrument the wait instead: record every call to `_settle_pending_function_tool_tasks`, the single bounded settle primitive both `_drain_cancelled_function_tool_tasks` and `_wait_pending_function_tool_tasks_for_timeout` funnel through, and require that parent cancellation never reaches it. The parked cleanup and hang guard stay, so an unbounded wait is still caught too. `test_parent_cancellation_does_not_report_tool_failure_as_background_error` waited on an event the tool set right after `asyncio.sleep(0)`, which only proved the tool raised at some point. The failure was not ordered after cancellation, and the negative assertion held whether or not the detached-task done callback ever ran. Raise the `ValueError` from the tool's cancellation cleanup so it cannot exist before the parent detaches the task, and rendezvous on `_consume_function_tool_task_result` -- the callback that makes the reporting decision -- so the negative assertion only runs once that path has executed. Teeth, each break failing only its own test: - Draining cancelled tool tasks before re-raising parent cancellation: old test passed, new test fails on the recorded settle call. - Reporting detached exceptions with the cleanup policy instead of the parent-cancelled policy: both fail (this one was already covered). - Dropping the detached-task reporting policy entirely: old test passed, new test fails because the callback never runs. `_flush_loop_callbacks` is gone: the rendezvous replaces it.
|
Both findings land. Fixed in the follow-up commit. On the hang guard carrying the assertion: I tried the state-transition option first and convinced myself it can't work here — the detached tool task's wakeup and the parent's wakeup are adjacent callbacks in the same loop round, so nothing causally observable separates "did not wait" from "waited briefly"; the only difference is elapsed time. So I took the instrumentation route instead. The test now records every call to Verified against your exact scenario: adding a bounded A caveat I'd rather state than hide — an ad-hoc inline On the failure ordering: the One correction for the record, since I checked before changing it. I couldn't reproduce the specific "reports detached But the underlying point is right and, if anything, understated: the old test also passed when I removed the detached-task reporting policy entirely, because it only ever had a negative assertion and never proved the callback ran at all. The new version fails that too. Load re-run (24 burners, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de44c3a5a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try: | ||
| original_consume(task, **kwargs) | ||
| finally: | ||
| background_callback_ran.set() |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new monkeypatch sets background_callback_ran for every _consume_function_tool_task_result call. In the parent-cancellation path, the runtime attaches one consume callback to the cancelled outer tool task and another to the shielded invoke task; the outer task’s callback can run first and set this event while the invoke task is still finishing cancellation cleanup. In that ordering the test restores the exception handler and performs the negative assertion before the callback that would report the ValueError("boom") has run, so a regression that reports detached invoke-task exceptions can still pass. Gate the event on the specific non-cancelled invoke task / expected exception path instead of any consume callback.
Useful? React with 👍 / 👎.
| # cancellation has detached this tool. Failing any earlier lets the runtime | ||
| # consume the exception from an already-completed task, and the background | ||
| # reporting path asserted below is never reached. | ||
| raise ValueError("boom") from None |
There was a problem hiding this comment.
Because this revision moves boom into cancellation cleanup, the failure now belongs to the shielded invoke task, which has not returned through _run_single_tool's except Exception wrapper; if the parent-cancel callback were wrong and reported normal Exceptions, the loop context would contain a raw ValueError("boom"), not the UserError filtered by the assertion below. That means the regression this test is trying to guard can still pass even after the invoke callback runs; assert that no parent-detached callback reports the raw invoke-task exception instead of filtering only the old wrapped shape.
Useful? React with 👍 / 👎.
|
Closing this PR in favor of #4193 |
2 participants
Summary
Follow-up to #4165. Two parent-cancellation tests still decided pass/fail from wall-clock deadlines smaller than the runtime's own bounds, so a loaded machine could fail them even when the runtime behaved correctly.
test_parent_cancellation_does_not_wait_for_tool_cleanupasserted the parent settles within 100ms, but_FUNCTION_TOOL_CANCELLED_DRAIN_SECONDSalone allows the runtime 250ms — the deadline was below the worst case it was supposed to tolerate, and it carried the whole assertion. The ordering is now established with events: wait until the tool has parked inside its cleanup, then assert the parent surfaces cancellation while that cleanup is still blocked. The remaining timeouts only detect a hang.test_parent_cancellation_does_not_report_tool_failure_as_background_errorended with two bareasyncio.sleep(0)calls and a negative assertion, so it could pass simply because nothing had run yet. It now waits for the tool to actually fail — proving the intended interleaving happened — and then drains the loop's callback queue, since the report is delivered from a done callback.Tests only; no runtime behavior changes.
Test plan
uv run pytest tests/test_run_step_execution.py -k parent_cancellationpasses.execute()makes..._does_not_wait_for_tool_cleanupfail withTimeoutError;_parent_cancelled_task_exception_messagereportExceptionwith the cancellation-cleanup message makes..._does_not_report_tool_failure_as_background_errorfail immediately.Each break fails only its own test, so the two are independently pinned.
bash .agents/skills/code-change-verification/scripts/run.sh→ all commands passed.Issue number
N/A — follow-up to #4165.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR