chore: API Server - Return 409 instead of 500 when container execution not yet available#297
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
01a0cb1 to
81eeead
Compare
e123ec2 to
176effc
Compare
| ): | ||
| with contextual_logging.logging_context(execution_id=exc.execution_node_id): | ||
| _logger.info( | ||
| "Container execution not ready (status=%s)", exc.execution_status |
There was a problem hiding this comment.
nit: We can bake execution status into logging_context above
Are you sure this can happen? At first glance this looks like it would be a bug. |
Would 404 be more appropriate? The container sub-resource does not exist (yet). |
| request: fastapi.Request, exc: errors.ContainerExecutionNotReadyError | ||
| ): | ||
| with contextual_logging.logging_context(execution_id=exc.execution_node_id): | ||
| _logger.info( |
There was a problem hiding this comment.
Not sure we need to log this. This is a common occurrence.
There was a problem hiding this comment.
Thank you for letting me know. I was on the fence about the log.
There was a problem hiding this comment.
Good call — removed the log line (and the now-unused _logger/logging import). It was a common, expected occurrence and the 409 response is self-describing.
Ark-kun
changed the title
I was thinking 404 but it can be confused with the execution entity / DB record not being found. 409 is an alternative that could be used to say the route was valid, and found a record, but could not produce the intended result |
I shall verify |
This has been disproven and the statement will be removed from this PR description. Tangle UI actively polls for container execution information for subgraph nodes when said data will never exist. An issue has been created in Tangle UI: TangleML/tangle-ui#2539 |
…able An ExecutionNode exists before its ContainerExecution row is created and linked by the orchestrator. During that window (status PENDING, no pod yet), the container_state / container_log / stream_container_log endpoints raised RuntimeError / ApiServiceError, surfacing as uncaught 500s and Bugsnag noise. Add ContainerExecutionNotReadyError (distinct from ItemNotFoundError, which means the execution itself is absent), raise it at all three sites, and map it to a 409 Conflict with a machine-readable body. The dedicated handler takes precedence over the catch-all Exception handler, so Bugsnag is not notified. Genuine 'execution does not exist' still returns 404.
176effc to
45b7a02
Compare
|
2 participants
Closes #296
Problem
Three read endpoints return 500 + a Bugsnag notification when an
ExecutionNodeexists but has no linkedContainerExecutionrow:GET /api/executions/{id}/container_stateGET /api/executions/{id}/container_logGET /api/executions/{id}/stream_container_logThey gate on the relationship (
if not execution.container_execution) and raisedRuntimeError/ApiServiceError, which had no handler inapi_router.py, so they fell through to the catch-allExceptionhandler → 500 + Bugsnag.A node can legitimately have no container row in several committed states —
container_execution_statusandcontainer_executionare independent columns with no invariant coupling them:Nonepermanently. The frontend polls these (its fetch fires on!status), so this is the dominant, guaranteed source of the noise.QUEUED/WAITING_FOR_UPSTREAMbefore the orchestrator launches; hit by direct API callers (tangle-deploy, operator scripts).PUT /api/admin/execution_node/{id}/status) — can set any status, includingPENDING, without a container link.In all of these the condition is expected, not a server fault. The recurring 500s also buried genuine
execution does not exist404s under expected-state noise. (Note: the orchestrator's normal launch/reuse paths do not produce this — they write status and link atomically in one transaction; full analysis in the investigation doc.)Changes
errors.py— newContainerExecutionNotReadyErrorcarryingexecution_status, distinct fromItemNotFoundError(which means the execution itself is absent).api_server_sql.py— raise it at all three sites when the node exists butcontainer_executionisNone, unifying the previousRuntimeError+ApiServiceError. The existing gracefulSYSTEM_ERROR-with-no-row path is unchanged.api_router.py— handler mapping it to 409 Conflict with a machine-readable body:{"reason": "container_not_ready", "execution_status": "<status-or-null>", "message": "..."}.The fix keys off the container link (not the status), so it covers every case above regardless of the node's status.
System impact
The dedicated
ContainerExecutionNotReadyErrorhandler is more specific than the catch-allExceptionhandler inapi_server_main.py, so it takes precedence and Bugsnag is not notified on this path. Genuineexecution does not existstill returns 404.Tests
Two focused service-level tests: a node with no container row raises
ContainerExecutionNotReadyError(withexecution_statuspopulated), and a genuinely absent execution still raisesItemNotFoundError.