Use async DB session for Execution API task-instance heartbeat#67800
Use async DB session for Execution API task-instance heartbeat#67800Dev-iL wants to merge 1 commit into
Conversation
455e42b to
8d811ea
Compare
I think the agent made these up :) |
I failed to edit the description after opening the mentioned tracking issue - here it is: #67801 |
8d811ea to
3d5f024
Compare
3d5f024 to
369d09f
Compare
5610f95 to
ad09f2d
Compare
|
@ashb Could you please take another look and see if your concerns were addressed? |
|
@Dev-iL I feel very uncomfortable merging this without the swap away from asyncpg first as it would essentially break all task execution for anyone using pgbouncer unless they happen to notice the change in the docs. Either we need to change the asyncpg migration to disable the prepared statments by default, or do #67801 first. Given 3.3.0 is about to enter the Beta and RC stage we need to be careful about not merging "half-breaking" changes to main. |
|
ad09f2d to
d523f0a
Compare
06ab268 to
371529e
Compare
|
@ashb How about now? |
Convert ti_heartbeat from the synchronous SessionDep to the async AsyncSessionDep, adopting the async metadata engine that already ships in Airflow 3.x. The route's behavior is unchanged: same 204/404/409/410 responses, same fast-path UPDATE / SELECT ... FOR UPDATE fallback, same last_heartbeat_at semantics, no version bump. Make AsyncSessionDep function-scoped so the async session commits before the response is sent, matching the synchronous SessionDep. FastAPI's default "request" scope for yield-dependencies commits after the response is sent, so a failed commit would roll back after the worker already received a 204 success. A regression test forces a commit failure and asserts the route surfaces an error rather than a silent 204. Heartbeat's async writes exposed a test-harness issue on Postgres: the async engine binds its connection pool to the event loop that created it, while the test harness builds a fresh app and loop per test, so a pooled connection from a prior test's closed loop was reused. Add a reconfigure_async_db_engine autouse fixture to the heartbeat tests that rebuilds the async session per test, mirroring the existing workaround in TestWaitDagRun. Document the asyncpg + transaction-mode pgbouncer prepared-statement caveat in the sql_alchemy_connect_args_async config reference and the PGBouncer setup guide (set statement_cache_size=0 and prepared_statement_cache_size=0); switching the default async driver to psycopg3 is tracked separately.
371529e to
083e686
Compare
Depends on:
Related:
What & why
Converts the Execution API
ti_heartbeatroute from the synchronousSessionDepto the asyncAsyncSessionDep, adopting the async metadata engine that already ships in Airflow 3.x (create_async_engine,async_sessionmaker,AsyncSessionDep,create_session_async).Heartbeat is the highest-QPS write path in the system (one call per running task per heartbeat interval, per worker), so it is a meaningful first production route to run on the async engine. The goal of this PR is behavioral parity with zero regression — not a throughput change. It follows the same low-blast-radius conversion pattern already merged for
GET /execution/variables/keys.What changed
execution_api/routes/task_instances.py):ti_heartbeatis nowasync defand takesAsyncSessionDep; the fast-pathUPDATE, the slow-pathSELECT … FOR UPDATE(.one()), the Task-Instance-History existence count, and the finalUPDATEare awaited. On the slow-path miss the route awaits its own TI-History count query and then delegates the 410/404 decision to the shared_raise_ti_not_in_live_tablehelper (see next bullet). No explicitcommit()is added — the async dependency commits on success and rolls back (releasing theFOR UPDATErow lock) on exception, mirroring the sync dependency exactly.synchronize_session=Falseandwith_for_update()are unchanged._raise_ti_not_in_live_table): Return 410 for stale id in set rtif API #68902 (merged intomainafter this branch was opened) refactored the inline 404/410 logic out of bothti_heartbeatandti_put_rtifinto this helper, which ran its own synchronoussession.scalar(...). That is incompatible with the async heartbeat session — ascalar(...)on anAsyncSessionreturns a coroutine, andif <coroutine>:is always truthy, so calling it unchanged would return410on every miss. The helper is therefore made session-agnostic: it no longer touches the DB and takes a keyword-onlyarchived_in_history: bool, raising410/404from that flag. Each caller runs its own count query in its own style — the async routeawaits it, the syncti_put_rtifroute does not — so the log/message/response logic stays centralized in one place.ti_put_rtif): updated to run the TI-History count query itself and pass the result asarchived_in_history=to the helper. This is the one change outside the heartbeat route; it is a mechanical adaptation to the new helper signature, forced by rebasing onto Return 410 for stale id in set rtif API #68902, and does not alter the rtif contract.versions/head/test_task_instances.py):UPDATEnow patchsqlalchemy.ext.asyncio.AsyncSessionwith an async interceptor (the route nowawaitsAsyncSession.execute); assertions are unchanged.reconfigure_async_db_engineautouse fixture toTestTIHealthEndpoint. The async engine binds its connection pool to the event loop that created it, while the test harness builds a fresh app and event loop per test; without this, a pooled connection from a prior test's closed loop is reused and fails (attached to a different loop). This is the same workaround already used byTestWaitDagRun.connect_argsrecipe. This PR therefore no longer adds a duplicate PgBouncer block to the setup guide (that block was dropped in the rebase); it keeps only a concise note on thesql_alchemy_connect_args_asyncconfig reference inconfig.yml, framed around the asyncpg opt-in and pointing operators at that section.max_connectionsfor both.Behavioral parity
No change to the endpoint contract: same
204success, same404/409(running-elsewhere / not-running) /410(cleared-and-archived) responses with identical detail payloads, same request signature. No Execution API version bump. The 404/410 branch now routes through the shared helper introduced by #68902 rather than an inline block, but the emitted responses are identical; the heartbeat and rtif tests (including the410cases) pass and are the parity proof.Testing
Heartbeat + rtif suite passes on SQLite (aiosqlite) after the rebase onto #68902, including both
410paths (test_ti_heartbeat_cleared_task_returns_410,test_ti_put_rtif_archived_ti_returns_410)Operational notes
sql_alchemy_conn_async = postgresql+asyncpg://…). It uses named server-side prepared statements, which break under transaction-mode PgBouncer, so it needssql_alchemy_connect_args_async = {"statement_cache_size": 0, "prepared_statement_cache_size": 0}. This PR documents that recipe.Note to reviewers
410for the rtif API), which introduced the shared_raise_ti_not_in_live_tablehelper. Resolving that rebase is why this heartbeat PR also touchesti_put_rtifand the helper's signature — see the "Shared helper" / "rtif route" bullets above. No rtif behavior changes.Event loop is closedmessage is logged during session teardown when the async engine is disposed (dispose_ormclosing the async engine synchronously). It appears on all three async drivers, is pre-existing engine disposal behavior unrelated to this change, and every run exits0. Tidying async-engine disposal is out of scope for this PR.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.8 following the guidelines
{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.