fix(agent-server): survive webhook rate limiting and reap stale run tasks#3991
Open
shanemort1982 wants to merge 2 commits into
Open
fix(agent-server): survive webhook rate limiting and reap stale run tasks#3991shanemort1982 wants to merge 2 commits into
shanemort1982 wants to merge 2 commits into
Conversation
…asks Fixes OpenHands#3842. Two defects compounded into permanently wedged conversations: 1. WebhookSubscriber._post_events retried on a fixed delay with no backoff, ignored Retry-After, ran overlapping volleys, and after exhausting retries only tried again when a NEW event arrived - so against a rate-limited downstream (the OpenHands app server rate-limits /api/v1/webhooks/events/* like any other route) delivery stalled forever and the queue trim dropped events permanently. 2. EventService.run() raised conversation_already_running whenever _run_task was alive, even with execution_status reporting non-RUNNING; a run task that never completes therefore blocked the conversation forever - the self-heal from OpenHands#3364 never fires because the task never reaches its cleanup tail. Webhook poster: serialise flushes; exponential backoff honouring Retry-After; exponentially growing cooldown (1s floor, 60s cap) instead of hammering; re-arm the flush timer on failure so delivery resumes without new events; prepend re-queued events to preserve delivery order. EventService.run(): when a live _run_task coexists with non-RUNNING status, start a staleness clock; reap the same task if still lingering past STALE_RUN_TASK_REAP_SECONDS (120s, well beyond the 30s-bounded wait_for_pending tail) and start the requested run. The task finally block now only clears its own task reference so a reaped task cannot clobber its replacement.
The failure cooldown meant events could accumulate in __call__ without passing through the failed-flush trim, growing the queue past max_queue_size (caught by test_webhook_queue_bounded_under_sustained_downstream_failure). Enforce the bound at enqueue as well. Also annotate the new test helper as list[Event] to satisfy pyright's invariance check on WebhookSubscriber.queue.
Collaborator
|
[Automatic Post]: It has been a while since there was any activity on this PR. @shanemort1982, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
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
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.
HUMAN:
Follow-up to my report in #3842, which VascoSch92 reproduced. We caught the trigger in the wild on our fleet: the app server rate-limits its own sandbox's webhook posts (429), the stock poster gives up, the app-side transcript freezes, and the lingering run task answers 409 forever with no self-heal. Fix validated end to end on a self-hosted box (evidence below) on top of the unit suites.
AGENT:
Why
Fixes #3842.
Two defects compound into the wedge:
WebhookSubscriber._post_eventsretries on a fixed delay with no backoff, does not honourRetry-After, fires overlapping volleys (each buffer-full flush posts concurrently), and after exhausting retries only tries again when a NEW event arrives. Against a rate-limited downstream (the OpenHands app server appliesInMemoryRateLimiter(requests=10, seconds=1)per client IP to ALL routes, including/api/v1/webhooks/events/...) the concurrent volleys trip the limiter, keep it tripped, and delivery stalls; the queue trim then drops events permanently. Production signature: hundreds ofWebhook post attempt 4 failed: 429/Failed to post events ... after 4 attemptsper second for 90+ minutes while the app-side transcript sat frozen (the agent's local store reached 8,622 events; the app side had 270).EventService.run()raisesconversation_already_runningwhenever_run_taskis alive, even whileexecution_statusreports non-RUNNING. A run task that never completes therefore wedges the conversation forever - the self-heal added in fix(agent-server): re-run input stranded in a finished run's cleanup tail #3364 never fires because the task never reaches its cleanup tail.Summary
Retry-After; after a failed volley enter an exponentially growing cooldown (1s floor, 60s cap) instead of hammering; re-arm the flush timer on failure so delivery resumes WITHOUT requiring a new event; preserve delivery order by prepending re-queued events.EventService.run(): when a live_run_taskcoexists with non-RUNNINGexecution_status, start a staleness clock; if the SAME task is still lingering on a later attempt pastSTALE_RUN_TASK_REAP_SECONDS(120s, comfortably beyond the 30s-boundedwait_for_pendingtail), cancel and reap it, then start the requested run. The task'sfinallynow only clears its own task reference so a reaped task cannot clobber a replacement run.How to Test
Commands run in a clean checkout with the patch applied:
uv run pytest tests/agent_server/test_webhook_subscriber.py tests/agent_server/test_event_service.py-> 150 passed. 5 new tests cover Retry-After/backoff with zero event loss and order preservation, cooldown queueing without posts, and the stale-run-task reaper (first observation never reaps; same lingering task past the grace period is cancelled and cleared; a different task restarts the clock). 5 existing assertions updated where the fixed-delay retry cadence intentionally changed to exponential.uv run ruff check/uv run ruff formaton the four touched files: clean.End to end on a self-hosted deployment (agent-server image built from this branch's changes applied to v1.30.0,
--target source; OpenHands app at current main). The app's global rate limit was tightened torequests=1/secondto force the storm, then a long multi-step streaming conversation was driven through the full app -> sandbox -> webhook path:Failed to post events to webhook ... after 4 attempts); the app-side transcript stalls until unrelated later events happen to re-trigger a flush - or forever when the storm persists (the production case above).