iralp commented on issue #69975:
URL: https://github.com/apache/airflow/issues/69975#issuecomment-6445299523
I would like to work on this and get consensus on the approach first.
The event is lost from memory if the corresponding database commit in the
session doesnt happen.
Unlike other task instances, Callback supervisor doesnt report its
failure/success outcome on process exit, and there's no heartbeat based sweeper
to move the Callback task from one state to another. So, it gets wedged in
queued/running state forever if the event is lost from the memory uncommitted.
**On #70030**
I think the instinct there was right, with two gaps. The `except Exception`
handler doesn't run for a SIGKILL or OOM; and restoring *all* events makes
task-event processing at-least-once, which worries me more — replaying a task
event can have unintended side effects. Replaying a *callback* event is a bare
state re-assignment and can't do that, so I'd scope the retry to the callback
lane rather than the whole buffer.
**Three options, as I see them**
The first two are the ones named when #70030 was self-closed
("drain-after-commit or DB-reselectability"); the third I don't think has come
up yet.
**A. Don't remove a callback event before the state that replaces it is
committed.** Handle the callback entries first, in their own transaction, then
drop those keys, and only then run today's drain and task handling unchanged.
If the callback commit fails, return before the drain — neither lane's events
are removed and both retry next loop. No migration, no public API change, and
it inverts today's coupling: instead of one bad callback rolling back every
executor's events, a callback failure at worst delays the task lane by one
scheduler loop. Doesn't survive a scheduler SIGKILL, since the buffer is memory.
**B. DB re-selectability.** A periodic sweep that re-queues or fails
callbacks stranded in `QUEUED`/`RUNNING`, in the spirit of the existing
stuck-in-queued and heartbeat sweeps. It needs a schema change — there's no
timestamp recording when the row entered its current state — and it would have
to re-run the callback rather than re-read its outcome, since nothing persists
the executor-side handle for a callback the way `external_executor_id` does for
a task instance. Its distinct value is that it's the only option that recovers
when nothing was ever reported at all: node lost, pod evicted, supervisor
killed before its outcome reached the executor's backend.
**C. Have the supervisor report its own terminal state.** It already holds
an authenticated Execution API client and already exchanges its token on
startup; it knows the outcome and currently discards everything but the exit
code. Reporting it would make the state durable at the moment it's known,
demote the executor event to a reconciliation hint exactly as it is for task
instances, and preserve the real error instead of an exit code. Costs a
version-gated endpoint, an SDK method and a scope declaration. Survives a
scheduler crash and re-runs nothing, but doesn't cover the supervisor dying
before it can report — that case needs B.
**Questions**
1. Which direction do you want? They're layers rather than alternatives:
**A** closes the reported bug, **C** makes the outcome durable at the source,
**B** is the backstop for when nothing is reported at all. I'd like to start
with **A**, then we can decide whether B and C are worth the extra effort.
2. For **A**, is delaying task-event processing by one scheduler loop an
acceptable response to a callback-lane DB error? Nothing is lost either way,
but it's a deliberate coupling and I'd rather have it confirmed than assume it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
