Validate trigger events in Openai deferrable tasks - #69506
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Three real defects here, and the first is the one that matters most.
A batch cancelled while the task was deferred completed successfully with no results, because execute_complete only raised on status == "error". The non-deferrable path has raised for CANCELLED/CANCELLING since wait_for_batch was fixed, so the same operator disagreed with itself depending on whether deferrable was set. Silent success on a cancelled job is about the worst failure shape there is.
Moving from a deny-list (== "error" raises) to an allow-list (!= "success" raises) is the right correction, and it is what makes unknown statuses fail loudly rather than being read as success — the general form of the bug rather than just the reported instance.
The stray error yield sitting outside the if/elif chain was worth catching too. In practice the triggerer consumes the first event and closes the generator, so it was probably latent rather than live, but it was plainly wrong.
I checked the thing most likely to break under the new allow-list: execute_complete dereferences event["message"] for every non-success event. Every branch of this trigger — success, cancelled, both error paths, and the exception handler — sets message, so that is safe.
One small inconsistency, worth a follow-up rather than holding this: the validator checks status but not message, and message is exactly what the caller dereferences next. A custom or version-skewed trigger emitting {"status": "cancelled"} alone would KeyError — precisely the "crash opaquely" failure the validator exists to prevent. event.get("message", event) at the raise site would close it.
I have added a note to the provider changelog rather than letting this land silently. Dags whose batches get cancelled will now fail where they previously went green, and that is worth operators reading before they upgrade, even though the earlier green runs produced no output.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
9471e40 to
b0ca49d
Compare
A deferred batch that was cancelled previously completed successfully with no results, so operators upgrading need to know their tasks will start failing where they used to go green.
b0ca49d to
8a8d3d2
Compare
Two PRs merged three hours apart on 2026-08-01 collided semantically: #69506 added a test using the class attribute END_TIME, and #69534 renamed that attribute to LEGACY_END_TIME while branched off a main that predated #69506. Git merged both cleanly, so the dangling reference reached main unnoticed and every job that collects the OpenAI provider tests now fails. The case asserts that a terminal batch emits exactly one event, which has nothing to do with the deprecated wall-clock deadline, so it moves to the timeout constant that the rest of the behavioural cases use rather than to LEGACY_END_TIME. The end_time path stays covered by test_serialization_with_legacy_end_time.
OpenAITriggerBatchOperator.execute_completeused the trigger event without validating it, branching deny-list style (onlystatus == "error"raised, everything else succeeded):cancelledwas marked SUCCESS: the provider's ownOpenAIBatchTriggeremits{"status": "cancelled"}when the batch is cancelled while the task is deferred, but onlyerrorraised — so the task went green with no results. The non-deferrable path (OpenAIHook.wait_for_batch) has raised forCANCELLED/CANCELLINGsince feat(providers/openai): support batch api in hook/operator/trigger #41554, so the two modes of the same operator disagreed.event=None: crashed with an opaqueTypeError: 'NoneType' object is not subscriptable.status(triggerer/worker version skew, custom trigger): fell through into the success path.Same fix shape as the anthropic provider in #69379: a validate_execute_complete_event() helper plus allow-list branching (status != "success" raises).
Also fixes a missing
elseinOpenAIBatchTrigger.run()that yielded a second, spuriouserrorevent after every terminal event (benign — only the first event is consumed — but incorrect).Was generative AI tooling used to co-author this PR?
Claude Code
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.