Skip to content

Use FastAPI exception handler for execution API database errors#67206

Open
ColtenOuO wants to merge 1 commit into
apache:mainfrom
ColtenOuO:api-replace-execution-todo-with-fastapi-handler
Open

Use FastAPI exception handler for execution API database errors#67206
ColtenOuO wants to merge 1 commit into
apache:mainfrom
ColtenOuO:api-replace-execution-todo-with-fastapi-handler

Conversation

@ColtenOuO
Copy link
Copy Markdown
Contributor

@ColtenOuO ColtenOuO commented May 19, 2026

The execution API had two routes (ti_run, ti_update_state) wrapping their session.execute calls in try/except SQLAlchemyError and converting to HTTPException(500, "Database error occurred"). The TODO in routes/task_instances.py flagged this for replacement with FastAPI's custom exception handlers.

This adds @app.exception_handler(SQLAlchemyError) alongside the existing catch-all in execution_api/app.py. The new handler logs the request path/method and returns {"detail": "Database error occurred"} with status 500 — the same shape the routes produced before. The per-route try/except blocks are removed and bodies dedented (which accounts for most of the line churn).

Scope note

The TODO was only on ti_update_state (around the call to session.execute(query) that writes the new state + audit Log). However, ti_run had the same try/except SQLAlchemyError boilerplate without a TODO, doing literally the same translation. Once the global handler exists, both local catches become redundant, so this PR removes both for consistency rather than leaving the codebase in a half-migrated state.

If reviewers prefer a strict TODO-only scope, I'm happy to revert the ti_run change and open a separate cleanup PR for it.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Copy link
Copy Markdown
Contributor

@SameerMesiah97 SameerMesiah97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me overall. Main thing I think is missing is a regression test exercising a real execution API route with a mocked SQLAlchemyError, mainly to verify the FastAPI handler registration and response contract end-to-end. CI needs to complete running too but I still think the above point applies regardless of that.

@ColtenOuO ColtenOuO force-pushed the api-replace-execution-todo-with-fastapi-handler branch from 5051c06 to 11c5554 Compare May 19, 2026 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants