Repository navigation
Keep HttpEventTrigger asset watchers polling after a failed request - #72376
Conversation
37cf0ab to
8a55e8c
Compare
An asset watcher that gives up on its first transient error is not watching anything, and a failure reported as a bare str(e) leaves no way to tell a 503 apart from a broken response_check callable. Because the exception was swallowed rather than raised, the triggerer had nothing to record either, so the traceback was lost at both layers. Retrying forever is the opposite failure mode, so the retry gives up after a bounded number of consecutive failures and lets the error reach the triggerer.
8a55e8c to
27f8867
Compare
A watcher riding out a flaky endpoint wrote a full traceback for every failed poll, while the escalation itself carried none of its own. The triggerer already routes its own record into the trigger's log, so the volume and the emphasis were both backwards. The effective failure tolerance is the cap times poll_interval rather than a fixed duration, which the parameter documentation now says.
The count alone does not tell a reader how long a watcher keeps trying, and describing it as the number of failures "tolerated" read one poll off from what the code does.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Nice change — I traced the before/after through the triggerer rather than taking the description on faith, and the motivating bug is if anything worse than described.
run() returns after the first exception, so cleanup_finished_triggers removes it from self.triggers on the next ~1s loop tick and update_triggers immediately recreates it, because the trigger row is still requested. A failing endpoint today therefore doesn't just stop the watcher — it turns it into a ~1 req/s hammer with poll_interval ignored entirely, since the only sleep sat on the success path. The table's "respawns the watcher ~1s later" is the steady state, not a one-off.
Raising is safe for watchers, which is the part I most wanted to confirm: Trigger.submit_failure only touches TaskInstance rows in DEFERRED state, so for an asset watcher it is a no-op — the trigger row survives and gets recreated. "The triggerer then restarts the watcher" in the new docs is accurate.
The control flow is right, including a subtlety that is easy to get wrong: yield event sits in the else: clause rather than inside try:, so an exception thrown in at the yield point (aclose → GeneratorExit, or athrow) is not eligible for the except Exception handler. A version that yielded inside the try would have swallowed it. Together with the sleep sitting outside the try, the cancellation claims hold up.
failures >= max matches the documented boundary, the reset-to-zero semantics match the "any completed poll resets the count" line, and ValueError over AirflowException is the right call — the wording even mirrors HttpAsyncHook's existing "Retry limit must be greater or equal to 1".
The tests are good. test_trigger_resets_failure_count_after_a_completed_poll is constructed so that a decrement-instead-of-reset bug would trip the cap of 3, which is exactly the failure a weaker test would miss.
Approving. Nits left inline, plus one thing that has no code line to hang off:
The description's "anything deferred on the trigger fails instead of hanging" is wrong, and it will outlive the PR as the commit message. Dependents already fail today: cleanup_finished_triggers appends to failed_triggers whenever details["events"] == 0, regardless of whether the task raised — so the current silent return submits a failure too, just with saved_exc = None and therefore traceback = None. The real improvement is that the traceback is now populated, not hang-vs-fail. Worth fixing before this is squashed.
One process note: the checks are split across two runs and the newer one has 5 cancelled jobs (cancelled, not failed — looks superseded). Worth a clean re-run so there is one unambiguous green run before merge.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
``:parama`` renders as literal text rather than a parameter entry, so poll_interval never appeared in the generated API reference. The dropped test assertions checked for the absence of a log call that run() does not make, so nothing could ever have failed them.
|
Thanks for the thorough review! I traced the Triggerer lifecycle as well and agree with your points. I’ve:
Thanks again! |
…pache#72376) * Keep HttpEventTrigger asset watchers polling after a failed request An asset watcher that gives up on its first transient error is not watching anything, and a failure reported as a bare str(e) leaves no way to tell a 503 apart from a broken response_check callable. Because the exception was swallowed rather than raised, the triggerer had nothing to record either, so the traceback was lost at both layers. Retrying forever is the opposite failure mode, so the retry gives up after a bounded number of consecutive failures and lets the error reach the triggerer. * Log one HttpEventTrigger traceback per escalation, not per failed poll A watcher riding out a flaky endpoint wrote a full traceback for every failed poll, while the escalation itself carried none of its own. The triggerer already routes its own record into the trigger's log, so the volume and the emphasis were both backwards. The effective failure tolerance is the cap times poll_interval rather than a fixed duration, which the parameter documentation now says. * Spell out the HttpEventTrigger failure cap in the parameter reference The count alone does not tell a reader how long a watcher keeps trying, and describing it as the number of failures "tolerated" read one poll off from what the code does. * Fix the HttpEventTrigger poll_interval docstring tag ``:parama`` renders as literal text rather than a parameter entry, so poll_interval never appeared in the generated API reference. The dropped test assertions checked for the absence of a log call that run() does not make, so nothing could ever have failed them.
2 participants
Summary
HttpEventTriggerwrapped its whole poll loop in oneexcept Exceptionthat loggedstr(e)and returned, so the first error of any kind ended the generator. The watcher stopped firing even when the next poll would have succeeded, and since the only sleep sat on the success path the triggerer recreated it on every ~1s loop — polling the endpoint at roughly 1 req/s withpoll_intervalignored entirely.poll_intervalmax_consecutive_failuresconsecutive failures and raiseDependents already failed before this change — a trigger that exits without yielding lands in
failed_triggerswhether it returned or raised — so what raising adds is the traceback thatsubmit_failureused to record asNone.The default of 10 is picked against the default
poll_intervalof 60 seconds: roughly ten minutes of continuous failure before escalating. Effective tolerance is that product rather than the count, which the parameter documentation now states.Before / after
Both versions of
run()driven through the same scenarios withasyncio.sleepstubbed:poll_intervalapartresponse_checkraisesCancelledErrorpropagatesTests
test_trigger_on_post_with_datarelied on the swallowed exception to end the generator, so it now drives one successful poll instead.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines