Fix webhook late acknowledgement for python-interpreter#18
Merged
Conversation
The webhook endpoint now acknowledges with 202 Accepted and runs the agent in a background asyncio task, rather than waiting for the agent loop to complete before responding. This aligns with webhook semantics where the caller does not expect a meaningful response body and also conforms with the AFM specification.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughWebhook handling changed from executing agents synchronously and returning results to scheduling agent execution as background tasks and immediately responding with HTTP 202 Accepted and Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookRouter
participant TaskScheduler
participant AgentWorker
Client->>WebhookRouter: POST /webhook (payload)
WebhookRouter->>TaskScheduler: Schedule background agent run (asyncio.create_task)
WebhookRouter-->>Client: 202 Accepted {"status":"accepted"}
TaskScheduler->>AgentWorker: Run agent in background
AgentWorker->>AgentWorker: Execute AFM/template, call LLM, log result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
python-interpreter/packages/afm-core/src/afm/interfaces/webhook.py (1)
255-261: Consider storing background task references for graceful shutdown.The fire-and-forget pattern works for the immediate acknowledgement goal, but agent execution tasks created here are not tracked. If the server shuts down while tasks are running, they will be abruptly cancelled without cleanup.
Consider storing task references (e.g., in
app.state) and awaiting/cancelling them in the lifespan shutdown handler, similar to howsubscription_taskis handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/src/afm/interfaces/webhook.py` around lines 255 - 261, The background agent task created in _run_agent_in_background is currently fire-and-forget and not tracked; modify the code that spawns this coroutine to create an asyncio.Task, store its reference (e.g., append to a list on app.state such as app.state.agent_tasks), and ensure the app lifespan shutdown handler cancels and awaits these tasks similar to how subscription_task is managed; update any code that calls _run_agent_in_background (or the caller that creates the task) to push the Task into app.state.agent_tasks and add cleanup logic in the existing shutdown/cleanup routine to iterate, cancel, and await tasks to allow graceful shutdown and cleanup.python-interpreter/packages/afm-core/tests/test_webhook_integration.py (2)
143-171: Consider verifying all background tasks completed.The test validates that 3 requests return 202, but doesn't verify that all 3 agent executions actually completed. Since
FakeListChatModelhas exactly 3 responses configured, you could verify the model's response queue was exhausted.♻️ Optional: Add completion verification
# Let all background tasks complete await asyncio.sleep(0.2) + + # Verify all 3 agent runs consumed the fake LLM responses + # FakeListChatModel cycles through responses, so if we got here without error, + # the tasks completed (or we can check internal state if exposed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py` around lines 143 - 171, After sending the 3 webhook requests in test_webhook_multiple_payloads, assert that all background agent runs actually completed by verifying FakeListChatModel consumed its configured responses (e.g., check its call count equals 3 or that its responses queue is empty) rather than only asserting HTTP 202; update the test to perform this check after the asyncio.sleep so you fail if any agent run didn't execute. Ensure you reference the FakeListChatModel instance (fake_llm) used to construct the LangChainRunner to perform the verification.
80-81: Fixed sleep durations may cause flaky tests in CI.Using
asyncio.sleep(0.1)to wait for background tasks can be unreliable under load or in slower CI environments. Consider either:
- Increasing the sleep duration with a safety margin (e.g., 0.5s)
- Polling with timeout for a condition that indicates task completion
- Exposing a mechanism to await pending tasks in test mode
Same concern applies to lines 127 and 171.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py` around lines 80 - 81, Replace the fragile fixed sleeps (await asyncio.sleep(0.1)) used to wait for background work with a deterministic wait: either poll for a clear completion condition with a timeout (e.g., use asyncio.wait_for + a small polling loop that checks the expected state/flag or mock call count) or expose/await the background task directly (e.g., an await_pending_tasks() test helper or awaiting an asyncio.Event set by the background task). Update the three occurrences of asyncio.sleep(0.1) in the tests to use this polling-with-timeout or awaiting mechanism (or, as a short-term fallback, increase to a safer duration like 0.5s) so tests no longer flake under CI load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python-interpreter/packages/afm-core/src/afm/interfaces/webhook.py`:
- Around line 255-261: The background agent task created in
_run_agent_in_background is currently fire-and-forget and not tracked; modify
the code that spawns this coroutine to create an asyncio.Task, store its
reference (e.g., append to a list on app.state such as app.state.agent_tasks),
and ensure the app lifespan shutdown handler cancels and awaits these tasks
similar to how subscription_task is managed; update any code that calls
_run_agent_in_background (or the caller that creates the task) to push the Task
into app.state.agent_tasks and add cleanup logic in the existing
shutdown/cleanup routine to iterate, cancel, and await tasks to allow graceful
shutdown and cleanup.
In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py`:
- Around line 143-171: After sending the 3 webhook requests in
test_webhook_multiple_payloads, assert that all background agent runs actually
completed by verifying FakeListChatModel consumed its configured responses
(e.g., check its call count equals 3 or that its responses queue is empty)
rather than only asserting HTTP 202; update the test to perform this check after
the asyncio.sleep so you fail if any agent run didn't execute. Ensure you
reference the FakeListChatModel instance (fake_llm) used to construct the
LangChainRunner to perform the verification.
- Around line 80-81: Replace the fragile fixed sleeps (await asyncio.sleep(0.1))
used to wait for background work with a deterministic wait: either poll for a
clear completion condition with a timeout (e.g., use asyncio.wait_for + a small
polling loop that checks the expected state/flag or mock call count) or
expose/await the background task directly (e.g., an await_pending_tasks() test
helper or awaiting an asyncio.Event set by the background task). Update the
three occurrences of asyncio.sleep(0.1) in the tests to use this
polling-with-timeout or awaiting mechanism (or, as a short-term fallback,
increase to a safer duration like 0.5s) so tests no longer flake under CI load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb07235c-9795-4c64-a797-e62eb54b112d
📒 Files selected for processing (3)
python-interpreter/packages/afm-core/src/afm/interfaces/webhook.pypython-interpreter/packages/afm-core/tests/test_webhook.pypython-interpreter/packages/afm-core/tests/test_webhook_integration.py
RadCod3
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python-interpreter/packages/afm-core/tests/test_webhook_integration.py (1)
188-229: Avoid the 5.5s wall-clock wait in this test.This proves the right behavior, but it makes the integration suite much slower than necessary and still depends on real timing. An event-gated fake model gives you the same coverage without sleeping for 5 seconds on every run.
One way to make it deterministic and fast
- agent_delay = 5.0 # seconds the mock LLM will sleep call_count = 0 + started = asyncio.Event() + release = asyncio.Event() class SlowFakeLLM(FakeListChatModel): async def _agenerate( self, messages: list[BaseMessage], @@ ) -> ChatResult: nonlocal call_count call_count += 1 - await asyncio.sleep(agent_delay) + started.set() + await release.wait() return await super()._agenerate( messages, stop=stop, run_manager=run_manager, **kwargs ) @@ assert response.status_code == 202 - # The response must arrive well before the agent delay completes - assert elapsed < 2.0, ( - f"Webhook responded in {elapsed:.2f}s; expected fast ack (<2s) " - f"while agent runs for ~{agent_delay}s in the background" - ) + assert elapsed < 0.5 + await asyncio.wait_for(started.wait(), timeout=1.0) - # Wait for the background agent to finish and verify it actually ran - await asyncio.sleep(agent_delay + 0.5) + release.set() + await asyncio.sleep(0) assert call_count == 1, "Slow mock LLM should have been invoked exactly once"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py` around lines 188 - 229, The test currently slows CI by making SlowFakeLLM._agenerate await asyncio.sleep(agent_delay) and then the test does await asyncio.sleep(agent_delay + 0.5); instead, replace the sleep-based wait with an asyncio.Event: add an event instance in the test (e.g., agent_done = asyncio.Event()), set agent_done.set() inside SlowFakeLLM._agenerate after incrementing call_count (or at the end of the method), and in the test wait for agent_done.wait() with an explicit short timeout (await asyncio.wait_for(agent_done.wait(), timeout=1.0)) to deterministically verify the background agent ran without a long wall-clock sleep; keep the same assertions around response timing and status_code and keep identifiers SlowFakeLLM, _agenerate, call_count, and agent_delay for locating the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py`:
- Around line 149-176: The test test_webhook_multiple_payloads currently only
asserts HTTP 202 responses but must also assert the three background agent runs;
modify the test to replace the blind await asyncio.sleep(...) with an explicit
assertion that the FakeListChatModel (fake_llm) observed three invocations (or
that LangChainRunner produced three results), e.g., poll or await until fake_llm
call count / consumed responses equals 3, verifying each response matches
"Processed event 1/2/3" (use FakeListChatModel and
LangChainRunner/create_webhook_app references to locate where to inspect the
model or runner state).
- Around line 112-134: The test races on a fixed sleep; replace the 100ms sleep
by signaling completion from the agent call: in the test create an asyncio.Event
(or similar) and have the tracking_agenerate wrapper set that event (after
appending to received_prompts) so the test can await event.wait() instead of
asyncio.sleep; tie this change to tracking_agenerate / fake_llm._agenerate and
await the event before asserting len(received_prompts) == 1 to ensure the
background task finished.
---
Nitpick comments:
In `@python-interpreter/packages/afm-core/tests/test_webhook_integration.py`:
- Around line 188-229: The test currently slows CI by making
SlowFakeLLM._agenerate await asyncio.sleep(agent_delay) and then the test does
await asyncio.sleep(agent_delay + 0.5); instead, replace the sleep-based wait
with an asyncio.Event: add an event instance in the test (e.g., agent_done =
asyncio.Event()), set agent_done.set() inside SlowFakeLLM._agenerate after
incrementing call_count (or at the end of the method), and in the test wait for
agent_done.wait() with an explicit short timeout (await
asyncio.wait_for(agent_done.wait(), timeout=1.0)) to deterministically verify
the background agent ran without a long wall-clock sleep; keep the same
assertions around response timing and status_code and keep identifiers
SlowFakeLLM, _agenerate, call_count, and agent_delay for locating the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 292692c3-6832-4e22-a8ab-ce80ae191b32
📒 Files selected for processing (1)
python-interpreter/packages/afm-core/tests/test_webhook_integration.py
MaryamZi
reviewed
Apr 9, 2026
MaryamZi
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request refactors the webhook interface to process incoming webhook payloads asynchronously, returning an HTTP 202 Accepted response immediately rather than waiting for agent execution to complete.
Part of #17
Summary by CodeRabbit
Webhook Handler Updates
Tests