fix: persist total_components count for EI analysis when job terminates - #2570
Conversation
Reviewer's GuideThis PR ensures exploit intelligence jobs persist an accurate total_components count when they terminate, and tightens tests around job detail fields so the API returns consistent metadata for completed and failed analyses. Sequence diagram for persisting total_components on EI job completionsequenceDiagram
participant RunnerPolling as poll_for_product_result
participant EiService as ExploitIntelligenceService
participant DB as Database
RunnerPolling->>EiService: fetch_components(job_id, db)
EiService-->>RunnerPolling: components (Vec<ExploitIntelligenceComponent>)
RunnerPolling->>RunnerPolling: total = Some(components.len() as i32)
alt has_finding or all_excluded
RunnerPolling->>EiService: update_job_completed(job_id, total, db)
EiService->>DB: ActiveModel{status=Completed, total_components=total}.update(db)
else no findings and not all excluded
RunnerPolling->>EiService: update_job_failed(job_id, error_message, total, db)
EiService->>DB: ActiveModel{status=Failed, total_components=total}.update(db)
end
Sequence diagram for persisting total_components on single-component EI job completionsequenceDiagram
participant RunnerPolling as poll_for_result
participant EiService as ExploitIntelligenceService
participant DB as Database
RunnerPolling->>EiService: update_component_completed(component_id, finding, advisory_id, db)
EiService-->>RunnerPolling: component updated
RunnerPolling->>EiService: update_job_completed(job_id, Some(1), db)
EiService->>DB: ActiveModel{status=Completed, total_components=Some(1)}.update(db)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The propagation of
Some(1)for single-component jobs is repeated in multiple places; consider encapsulating this in a helper or constant to avoid duplication and make future changes to the default behavior easier. - When casting
components.len()toi32fortotal_components, it may be safer to use a fallible conversion (e.g.,try_into()) or at least document the assumption about maximum component count to avoid potential overflow issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The propagation of `Some(1)` for single-component jobs is repeated in multiple places; consider encapsulating this in a helper or constant to avoid duplication and make future changes to the default behavior easier.
- When casting `components.len()` to `i32` for `total_components`, it may be safer to use a fallible conversion (e.g., `try_into()`) or at least document the assumption about maximum component count to avoid potential overflow issues.
## Individual Comments
### Comment 1
<location path="modules/exploit-intelligence/src/runner/polling.rs" line_range="292" />
<code_context>
- // Check whether any component actually has a finding.
let components = ei_service.fetch_components(job_id, db).await?;
+ let total = Some(components.len() as i32);
let has_finding = components.iter().any(|c| {
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting `components.len()` (usize) to `i32` can overflow for very large jobs.
The `as i32` cast will silently overflow if `components.len()` ever exceeds `i32::MAX`. To make this safer, consider clamping to `i32::MAX`, using `i32::try_from`, or using a wider integer type if the schema supports it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.ui_url(), | ||
| c, | ||
| ); | ||
| if s.total_components.is_none() { |
| tracing::warn!(job_id = %job_id, retry_count = job.retry_count, "failing job: max retries exhausted on claim"); | ||
| self.ei_service | ||
| .update_job_failed(job_id, "max retries exhausted", &tx) | ||
| .update_job_failed(job_id, "max retries exhausted", None, &tx) |
There was a problem hiding this comment.
maybe consider adding a code comment somewhere near here ...
// total_components is None here because failure occurs before
// (or independently of) component creation. If components already exist
// in DB (e.g. retry-exhausted during SPDX polling) stored
// total_components will remain NULL.
There was a problem hiding this comment.
On Phils recommendation, I'll follow up on non-functional changes like this in a separate PR so we don't have to wait the multiple hours for CI to finish before we can get this merged
|
|
||
| // Check whether any component actually has a finding. | ||
| let components = ei_service.fetch_components(job_id, db).await?; | ||
| let total = Some(components.len() as i32); |
There was a problem hiding this comment.
its unlikely any EI job would have 2b+ components ... though maybe add a source comment to inform future human/agents we ok with this
|
Successfully created backport PR for |
total_componentswas always set to NULL at job creation time. No code ever wrote it back to the DB during the polling/completion flow. The list endpoint masked this with a fallback that computed the count from component rows, but the details endpoint, which didn't have this fallback, returned the raw DB value of null.The fix:
update_job_completedandupdate_job_failednow accept an optionaltotal_componentsparameter and write it to the DB when provided. The polling code passes the known count (Some(1)for single-component,Some(components.len())for SPDX) at finalization time. The reason we do it when marking a job as failed/completed is that, to my understanding, EI doesn't give us a definitive list/number of components until EI itself marks a job as having terminated.Summary by Sourcery
Persist and surface accurate component counts for exploit intelligence analysis jobs when they complete or fail.
Bug Fixes:
Enhancements:
Tests: