Fix/fire and forget#723
Conversation
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
There was a problem hiding this comment.
Good direction — detaching the telemetry POST out of the reasoner's critical path is the right fix for #622, and the GC-safe idiom (_background_tasks.add + add_done_callback(discard)) is done correctly at all the sites. Two things holding back an approval, both inline.
Also: there are no tests for the new non-blocking behavior (the "covered by tests" box is unchecked), and this only fixes half of #622 — log_execution is still called synchronously on the event loop. Fine to scope that out, but say so in the PR/issue rather than leaving it silent. A ruff format pass would also clear the trailing-whitespace/indentation noise.
| reasoner_id, | ||
| payload_dict, | ||
| parent_execution_id=execution_context.parent_execution_id, | ||
| task = asyncio.create_task( |
There was a problem hiding this comment.
Detaching start/complete/error into independent tasks means the events can now hit the endpoint out of order — previously notify_call_start was awaited before the reasoner body ran, so "started" was guaranteed to post before "completed". If the control-plane telemetry orders by timestamp/status this is fine, but it's a real weakening of the prior contract — worth confirming the CP tolerates out-of-order start/complete and noting it in the PR.
There was a problem hiding this comment.
Internally task queue follow the FIFO property so if start pushed first then it will execute first unless and await called will be executed inside the first task which leads it to sleep and task after the first task will be executed. Here the only async code inside the all notify functions is fire_and_forget_update() method which might cause issue.
I have 2 approach to this:
- Convert
fire_and_forget_update()to a task also so that start, complete and error execute in order then their fire_and_forget_update also queued in order. - Instead of converting the entire notify function into a task we should only convert the respective
fire_and_forget_update()calls into task.
Please review these 2 approaches.
| parent_execution_id=execution_context.parent_execution_id, | ||
| ) | ||
| ) | ||
| self._background_tasks.add(task) |
There was a problem hiding this comment.
These background tasks are never drained on shutdown — stop() → _cleanup_async_resources() calls self.client.aclose() while notification POSTs may still be in flight, so the final complete/error event can be dropped if the reasoner returns right before teardown. Previously that terminal event was awaited, i.e. guaranteed delivered.
Suggest draining before the client closes: await asyncio.gather(*self._background_tasks, return_exceptions=True) (with a short timeout) in _cleanup_async_resources, ahead of aclose().
There was a problem hiding this comment.
Sorry for my mistake, I will work on it in the next commits.
Regarding the sync execution of |
- Wrapped all functions (`notify_call_start`, `notify_call_complete` and `notify_call_error`) into `asyncio.Task` so parent function don't have to await them. - Converted the `_emit_execution_transition_log` to sync as underlying code is sync and execute it to a separate thread.
…method using `asyncio.to_thread()` because of mismatch order. - Considering the `log_execution` inside the function is a lightweight function which don't takeover the `event_loop` for long time.
f9f2a41 to
f3e53c8
Compare
- Initially mock `Agent` is created through __new__ which by pass __init__ and it doesn't contain `_background_tasks` property. - I added `_background_tasks` property and added new assert in `test_cleanup_async_resources` test.
Summary
asyncio.Task.log_executionpart as it is as it seems lightweight no, needless to delegate it to a thread.Type of change
Test plan
./scripts/test-all.shTest coverage
I ran the Python SDK coverage test locally because this PR only changes Python-side code.
Command used:
Result:
1661 passed, 4 skipped, 36 deselected, 27 warnings
TOTAL coverage: 81%
Before asking for review, please confirm:
coverage-baseline.jsoninthis PR only if the removal caused a legitimate regression and I
called it out in the summary above.
Checklist
docs/DEVELOPMENT.md.
Related issues / PRs
Fixes #622