Repository navigation
Add Execution API endpoint for callback terminal state - #75253
Conversation
boring-cyborg
Bot
added
area:API
a4c315a to
3dd249f
Compare
fat-catTW
left a comment
fat-catTW
left a comment
There was a problem hiding this comment.
I think the scheduler fallback could race with the new state endpoint here.
The scheduler reads the callback without a lock, checks that it is still QUEUED/RUNNING, and later writes the executor event's terminal state. In between those operations, the callback supervisor can successfully report SUCCESS (including its output) through PATCH /callbacks/{callback_id}/state.
For example, the callback subprocess can succeed and report SUCCESS, while the outer supervisor subsequently exits abnormally and the executor emits a late FAILED event. The scheduler could then overwrite the supervisor's already-recorded result, despite the intended first-writer-wins behaviour.
Maybe we could lock the callback row before rechecking its state, or use a conditional update that only succeeds while the persisted state is still QUEUED or RUNNING and add a regression test for this interleaving?
fat-catTW
left a comment
fat-catTW
left a comment
There was a problem hiding this comment.
lgtm
Thanks for the PR :D
seanghaeli
left a comment
seanghaeli
left a comment
There was a problem hiding this comment.
Also a conflict to deal with, but otherwise looks ready!
ramitkataria
left a comment
ramitkataria
left a comment
There was a problem hiding this comment.
The conditional UPDATE looks like a nice fix for the race. A few smaller things my agent found on top of the inline comments:
- +1 to the suggestion on `routes/callbacks.py` about returning 204 when the row is already in the requested state, so retries don't log a traceback.
- Which executors is this mainly for? Edge seems to already pass callback events back to the scheduler since #74784, so maybe worth naming the ones that don't in the description. For those, a failed `/run` or an OOM-killed supervisor would still leave the row stuck, so the comment at `callback_supervisor.py:469` might want a tweak, and maybe a follow-up issue for a timeout-based cleanup of stuck `QUEUED`/`RUNNING` callbacks?
- `jwt_token_authentication.rst` still only lists `/run` for callbacks, so maybe worth adding `/state` there too.
- After rebasing onto #75689, it might be worth double-checking the 404 case in `test_state_validates_token_scope_and_sub`, since it depends on the order the auth dependencies resolve in.
8486c4e to
726dc55
Compare
o-nikolas
left a comment
o-nikolas
left a comment
There was a problem hiding this comment.
Agreed about a separate PR for the callouts from @ramitkataria
Callback state was written only by the scheduler, reading each executor's in-process event buffer. Executors whose workers report back over HTTP never reach that channel, so their callbacks sat in QUEUED forever. The executor-event path stays as a fallback, now narrowed to rows still in flight so a late event cannot overwrite an outcome the supervisor already reported, or the output captured with it. Replaces apache#66141, whose /run half landed separately in apache#71192. Co-authored-by: wjddn279 <[email protected]>
726dc55 to
917ad67
Compare
Callback state was written only by the scheduler, reading each executor's in-process event buffer. Executors whose workers report back over HTTP never reach that channel, so their callbacks sat in QUEUED forever.
The executor-event path stays as a fallback, now narrowed to rows still in flight so a late event cannot overwrite an outcome the supervisor already reported, or the output captured with it.
Replaces #66141, whose /run half landed separately in #71192. Most of this is @wjddn279's code from that PR ported on top of the current main, co-author credit should carry over.
Was generative AI tooling used to co-author this PR?
Inheriting the disclaimer from the parent PR:
claude code (opus 4.7) to make test codes and ci green{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.