## Context * The code mentioned in the parent issue is triggered by a request to the merge requests controller * This requests [currently polls](https://gitlab.com/gitlab-org/gitlab/-/blob/55151efbb58d45587fc44317f9fe95307fa1add3/app/controllers/projects/merge_requests_controller.rb#L588-592) while `pipeline&.active?` * It also polls while the report has a status of `parsing` [here](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/controllers/projects/merge_requests_controller.rb#L594-597) * that polling is set in the MR model [here](https://gitlab.com/gitlab-org/gitlab/blob/master/app/models/merge_request.rb#L1845-1852) * * As outlined in [this thread](https://gitlab.com/gitlab-org/gitlab/-/issues/390185#note_1374606930 "Use security_findings for security MR widget report comparison"), this is an issue 1. The current behavior correlates to ending the polling at step 3 in [the `Scan runs in a pipeline for a non-default branch` flow](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/development/sec/security_report_ingestion_overview.md#vulnerability-creation-from-security-reports) 2. We need to move the polling to last until after step 7 of the same flow (after the [`Security::StoreFindingsService`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/services/security/store_findings_service.rb) has completed its work ## Implementation plan The diff below is the basic idea, with two tweaks needed (mentioned in [this thread](https://gitlab.com/gitlab-org/gitlab/-/work_items/457851#note_1880760576)) 1. Use a redis cache to avoid database queries for every polling response - When we start a pipeline, we set a key in Redis with the pipeline state - When `Security::StoreFindingsService` is finished it clears that redis cache - If that redis key is present, return `:parsing` , else run the db query 2. We will need to check all scans instead of checking the first one as there can be more than one for given report type ### Allow report comparison service to return `:parsing` ```diff diff --git a/app/services/ci/compare_reports_base_service.rb b/app/services/ci/compare_reports_base_service.rb index ee687706b530..b654abf2590b 100644 --- a/app/services/ci/compare_reports_base_service.rb +++ b/app/services/ci/compare_reports_base_service.rb @@ -12,6 +12,9 @@ def execute(base_pipeline, head_pipeline) base_report = get_report(base_pipeline) head_report = get_report(head_pipeline) + + return parsing_payload(base_pipeline, head_pipeline) if base_report == :parsing || head_report == :parsing + comparer = build_comparer(base_report, head_report) { diff --git a/ee/app/services/ci/compare_security_reports_service.rb b/ee/app/services/ci/compare_security_reports_service.rb index 0f242200b450..bbbfcbbce530 100644 --- a/ee/app/services/ci/compare_security_reports_service.rb +++ b/ee/app/services/ci/compare_security_reports_service.rb @@ -15,7 +15,22 @@ def serializer_class end def get_report(pipeline) - Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: { report_type: [params[:report_type]], scope: 'all' }).execute + + return :parsing unless safe_to_generate_report?(pipeline) + + # we need to poll before this point until + # `Security::StoreFindingsService` is complete + Security::PipelineVulnerabilitiesFinder.new( + pipeline: pipeline, + params: { report_type: [params[:report_type]], scope: 'all' } + ).execute + end + + def safe_to_generate_report?(pipeline) + # Once `Security::StoreFindingsService` is complete + # `security_scan.succeeded!` is called in + # `Security::StoreScanService` + pipeline.security_scans.where(scan_type: params[:report_type]).first&.succeeded? end end end ``` <details><summary>notes from ~"workflow::refinement" </summary> ### `Security::StoreFindingsService` class The `Security::StoreFindingsService` class - takes a `security_scan`, a `report`, an an array of `deduplicated_finding_uuids` - the `report` has a list of `report_findings` - we loop over the `report_findings` in batches - do some data munging to convert each `report_finding` to a `Security::Finding` - associate each created `Security::Finding` to the `security_scan` - The service is "done" when all `security_findings` are processed and associated with the `security_scan` - If you call this `service` again with the same paramters, you will get a `error('Findings are already stored!')` - the error uses the following code to check if things are done: ```ruby def already_stored? security_scan.findings.any? end ``` ### - The [`Security::StoreScanService`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/services/security/store_scan_service.rb) is actually driving the `Security::StoreFindingsService` - When `Security::StoreFindingsService` has completed, it calls [`security_scan.succeeded!`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/services/security/store_scan_service.rb#L71-77) - this might be a hook we can check in the controller polling ### impl possibility 2: modify `Security::StoreFindingsService` to block polling <details><summary>description</summary> Based on how the `Security::StoreFindingsService` class works, we can maybe add a class method that utilizes the `already_stored?` method to block the polling on Questions TBD: - [ ] is `security_scan.findings.any?` *enough*? In theory after 1 `report_finding` is processed this would be `true`, even if there are more to process - [ ] if we need a more guaranteed check, we might need to create a new database table - Is there a generic "waiting for a batch job to finish" table? </details> ### impl possibility 3: Use `::Onboarding::ProgressService` to block polling <details><summary>description</summary> All of this happens within the [`Security::StoreScansWorker`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/workers/security/store_scans_worker.rb) That worker does call a [`record_onboarding_progress`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/workers/security/store_scans_worker.rb#L19) method. If this method is query-able, then we may just be good to go! I need to figure out what it actually does </details> </details>