kaxil commented on code in PR #67800:
URL: https://github.com/apache/airflow/pull/67800#discussion_r3359872625
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -761,10 +761,10 @@ def ti_skip_downstream(
]
),
)
-def ti_heartbeat(
+async def ti_heartbeat(
task_instance_id: UUID,
ti_payload: TIHeartbeatInfo,
- session: SessionDep,
+ session: AsyncSessionDep,
Review Comment:
`SessionDep` carries `scope="function"` but `AsyncSessionDep` doesn't
(`common.py:46` vs `:67`). In FastAPI 0.136.3 that kwarg decides which exit
stack the `yield` dependency commits on: `scope="function"` runs the commit
before `await response(...)`, the default ("request") runs it after -- the
response is sent between the function-stack and request-stack teardown in
`routing.py`.
Since this route has no explicit `commit()`, doesn't it return `204` before
`last_heartbeat_at` is persisted? A commit failure (asyncpg/PgBouncer drop)
would roll back after the worker already saw success, where the sync route
surfaced a `500`. Is that intended given the "zero regression" goal, or should
`AsyncSessionDep` also take `scope="function"`? Looks like #58524 added it to
`SessionDep` for this reason.
--
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]
