feat(preprod): Add NOT_RAN to SizeAnalysisState#107080
Conversation
b998efc to
3aa9a65
Compare
3aa9a65 to
5258347
Compare
| case PreprodArtifactSizeMetrics.SizeAnalysisState.NOT_RAN: | ||
| metrics.state = put.state | ||
| metrics.error_code = put.error_code | ||
| metrics.error_message = put.error_message |
There was a problem hiding this comment.
PutSize model missing NOT_RAN state variant
High Severity
The endpoint adds a case handler for NOT_RAN state, but the PutSize request model in launchpad.py was not updated to include a PutSizeNotRan variant. The PutSize union type only accepts PutSizeFailed | PutSizePending | PutSizeProcessing. Any API request attempting to set state to NOT_RAN will fail Pydantic validation before reaching the match statement, making this feature completely non-functional.
There was a problem hiding this comment.
PutSize - by design. Launchpad will never post back for a not_started analysis
| case PreprodArtifactSizeMetrics.SizeAnalysisState.NOT_RAN: | ||
| metrics.state = put.state | ||
| metrics.error_code = put.error_code | ||
| metrics.error_message = put.error_message |
There was a problem hiding this comment.
Duplicated match cases for FAILED and NOT_RAN states
Low Severity
The FAILED and NOT_RAN cases have identical bodies (setting state, error_code, and error_message). Python's pattern matching supports combining cases with |, which is already used elsewhere in the codebase (e.g., event_parser.py). These two cases could be combined into a single case like case ... FAILED | ... NOT_RAN: to reduce code duplication and ensure consistent handling.
| case PreprodArtifactSizeMetrics.SizeAnalysisState.FAILED: | ||
| errored_count += 1 | ||
| case PreprodArtifactSizeMetrics.SizeAnalysisState.NOT_RAN: | ||
| errored_count += 1 |
There was a problem hiding this comment.
There was a problem hiding this comment.
status check to be handled in follow up.
|
|
||
|
|
||
| SizeInfo = Annotated[ | ||
| SizeInfoPending | SizeInfoProcessing | SizeInfoCompleted | SizeInfoFailed, | ||
| SizeInfoPending | SizeInfoProcessing | SizeInfoCompleted | SizeInfoFailed | SizeInfoNotRan, | ||
| Field(discriminator="state"), | ||
| ] | ||
|
|
There was a problem hiding this comment.
Bug: The get_size_analysis_response function fails to handle the new NOT_RAN state, causing an incorrect 500 error with a misleading message instead of a 422 with error details.
Severity: MEDIUM
Suggested Fix
In the get_size_analysis_response function, add a conditional branch to explicitly handle the NOT_RAN state. This branch should return a 422 status code and include the specific error_code and error_message associated with the metric, similar to how the FAILED state is currently handled.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
src/sentry/preprod/api/models/project_preprod_build_details_models.py#L135-L141
Potential issue: The function `get_size_analysis_response` does not handle the newly
introduced `NOT_RAN` state for size analysis metrics. When a metric has this state, the
function bypasses existing checks for `PENDING`, `PROCESSING`, and `FAILED` states. It
then attempts to access the `analysis_file_id`, which is `None` for `NOT_RAN` metrics.
This results in a `SizeAnalysisResultsUnavailableError` being raised, leading to a 500
HTTP status code with the misleading message "Size analysis completed but results are
unavailable". The expected behavior is to return a 422 status code with a specific error
message explaining why the analysis was not run, such as `NO_QUOTA` or `SKIPPED`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| case PreprodArtifactSizeMetrics.SizeAnalysisState.FAILED: | ||
| errored_count += 1 | ||
| case PreprodArtifactSizeMetrics.SizeAnalysisState.NOT_RAN: | ||
| errored_count += 1 |
There was a problem hiding this comment.
NOT_RAN state causes inconsistent status check behavior
Medium Severity
The PR adds NOT_RAN handling in format_status_check_messages (incrementing errored_count), but the _compute_overall_status function in tasks.py wasn't updated. That function returns StatusCheckStatus.IN_PROGRESS for any state that isn't FAILED or COMPLETED. This causes NOT_RAN metrics to produce a status check that's permanently stuck at "in progress" while displaying a message saying "X app(s) errored" - an inconsistent state.
There was a problem hiding this comment.
status check to be handled in follow up.
Now we can not run size analysis for two legitimate reasons: - Not having enough quota - Skipping size because we only want to run build distribution It's important to be able to tell these apart in the UI. Currently we don't create a `PreprodArtifactSizeMetrics` row in this case which makes them hard to tell apart. This adds a new state for the row: `NOT_RAN` and also two new 'error' states: - NO_QUOTA - SKIPPED We make a new top level state to be able to easily tell this case apart from real failures but reuse error_code and error_message rather than introduce two new columns.
Now we can not run size analysis for two legitimate reasons:
It's important to be able to tell these apart in the UI.
Currently we don't create a
PreprodArtifactSizeMetricsrow in this case whichmakes them hard to tell apart.
This adds a new state for the row:
NOT_RANand also two new 'error' states:We make a new top level state to be able to easily tell this case apart from real failures
but reuse error_code and error_message rather than introduce two new columns.