feat: allow import/reimport to wait for deduplication to complete#15007
Open
valentijnscholten wants to merge 12 commits into
Open
feat: allow import/reimport to wait for deduplication to complete#15007valentijnscholten wants to merge 12 commits into
valentijnscholten wants to merge 12 commits into
Conversation
Introduce a third import/reimport post-processing execution mode and make it configurable per request and via the user profile. - async (default): dispatch post-processing to the background, respond immediately - async_wait (new): dispatch to the background but wait for deduplication to finish before responding, so scan_added notifications and the returned statistics reflect the deduplicated state - sync: run post-processing inline in the web process Replace the legacy UserContactInfo.block_execution boolean with an import_execution_mode CharField; a data migration maps block_execution=True to the 'sync' mode. wants_block_execution() now derives from the sync mode, preserving global foreground-execution semantics for all async tasks. Add a request field import_execution_mode (ImportScanSerializer/ ReImportScanSerializer) resolved against the profile (request override wins), and a deduplication_complete boolean in the import/reimport response. The post_process_findings_batch task now stores its result (ignore_result=False) so async_wait can join on it via AsyncResult.get(); the join is bounded by the new DD_IMPORT_ASYNC_WAIT_TIMEOUT setting.
notify_scan_added used the importer's in-memory finding lists, whose duplicate flag is stale because deduplication runs on separately-fetched instances. Once deduplication has completed (sync or async_wait, signalled by deduplication_complete), refresh the duplicate flag from the database and split each action list into "real" and duplicate findings: - findings_new -> net-new (excludes deduplicated) - findings_new_duplicate / findings_reactivated_duplicate / findings_untouched_duplicate - finding_count -> recomputed to exclude new/reactivated duplicates (so an all-duplicate import sends scan_added_empty) In plain async mode (dedup not awaited) the lists are left untouched, preserving historical behavior. Mail and webhook scan_added templates render the new duplicate sections. Note: import/reimport response statistics are already dedup-accurate once awaited — Test.statistics / Test_Import.statistics query Finding and annotate the duplicate count directly; only the notification used stale in-memory data.
Move wait_for_post_processing() ahead of the test_added notification dispatch (not just notify_scan_added) so both notifications sent during a fresh import are emitted after deduplication has completed in async_wait mode. The reimporter already orders the await before its single notification.
github-actions
Bot
added
New Migration
The mode governs whether the request waits for deduplication, so name it for that. Rename the UserContactInfo field, the request/serializer field, the ImporterOptions attribute and validator, the resolver, and the IMPORT_EXECUTION_MODE_* constants to DEDUPLICATION_EXECUTION_MODE_*. Add a RenameField migration (0271) rather than mutating the add/remove migrations introduced earlier on this branch.
…NC_WAIT_TIMEOUT, default 60s
…ion_mode for import dedup block_execution gates every async task (notifications, jira, grading, deletes, reindex, ...) via we_want_async, so it must remain. Restore it as the global "run all async tasks in the foreground" flag and make deduplication_execution_mode an independent field that only controls how import/reimport deduplication post-processing is dispatched and awaited. - Restore the block_execution field; wants_block_execution() reads it again. - resolve_deduplication_execution_mode() falls back to block_execution -> sync. - Replace the destructive remove migration (0270) with a seed-only data migration that maps block_execution=True -> deduplication_execution_mode 'sync'; keep both. - Restore fixtures, the profile form field, and the user detail view (both fields). - Revert the test sweep: tests that need global foreground use block_execution=True again; the dedicated deduplication_execution_mode tests are updated for the split.
…ort/reimport The API resolves deduplication_execution_mode in the serializer, but the UI import/reimport views built their context straight from the form and never resolved it, so UI imports silently defaulted to async regardless of the user's profile. Resolve it from the profile in both UI process_form paths.
valentijnscholten
changed the title
Option B keeps block_execution as a checkbox in the profile form, so the integration helper toggles id_block_execution again instead of the deduplication_execution_mode select (which no longer existed under that id).
Collapse the add/rename pair into a single AddField that creates deduplication_execution_mode with its final name, and keep the seed as a separate data migration, per the convention of keeping schema and data migrations apart. Drops the interim rename migration.
…E_LOCATIONS The CI 'true' matrix variant enables V3_FEATURE_LOCATIONS, where loading dojo_testdata.json (containing Endpoints) raises NotImplementedError. Decorate the test classes with @versioned_fixtures so the locations fixture is used in that mode, matching the other import/reimport test suites.
valentijnscholten
added
the
affects_pro
Covers the 'async_wait' execution mode: post-processing is dispatched to a background worker and the request joins on it before responding. The dedup queries run in the worker (separate connection), not the web request, so the only web-side delta over the plain async path is the post-dedup notification refresh SELECT (+1): 109->110 first import, 89->90 second. Does not use CELERY_TASK_ALWAYS_EAGER (that would run the task inline on the request connection and wrongly count worker queries). The dedup batch is dispatched async and the join (AsyncResult.get) is mocked to return instantly, simulating a finished worker. Adds an optional dedup_mode param to _deduplication_performance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While thinking about what a v3 import API could look like, I realized we're missing one piece of the puzzel in the current (re)import.
By default deduplication runs as an async celery task. The results are not awaited, so the (re)import process can finish before deduplication is complete.
Consequences are:
scan_addedcontains incorrect statisticsdeltastatitistics.With the recent optimizations on both import and deduplication the chances of these incorrect statistics has gone down. However the most confusing case happens when the deduplication is partly finished. In that case the statistics will contain some duplicates, but not all of them. Example report: #13777
Pro also suffers from this when not using the background-import. Also the pattern in this PR could be used for other Pro features like the current asynchronous prioritization.
The solution in this PR is to introduce a new
deduplication_execution_modeenum that controls deduplication execution:async (default): perform deduplication in the background, so not wait for the results.
sync (
block_execution==True): perform deduplication in the foreground/in-line with the (re)import.async_await: perform deduplication in the background and wait for the results.
The
async_awaitis the new option and could be the new default in the future. Theblock_executionuser profile variable is removed in this PR as it is obselete.Data is migrated. Using
block_execution==Truecould be used to achieve a similar result, but it would slow the import a lot more because deduplication would run in the foreground. It would also make everything synchronous inluding notifications, pushing to JIRA, etc causing further slowdowns.The new
deduplication_execution_modecan be set in the user profile, but also in the API requests.In most scenario's the await setting will not lead to significantly longer running import, may only 1 or 2 seconds extra.
The exception is on very busy/overloaded instances where the sync deduplication jobs are queueing up.
For this reason the maximum wait time is 1 minute. After 1 minute the (re)import no longer waits and returns statistics as-is.
A response variable
deduplication_completewill beFalsein this case.Details
deduplication_execution_mode(async/async_wait/sync) and overridable per request through adeduplication_execution_modefield on the import and reimport endpoints. The request value takes precedence over the profile, otherwise it falls back toasync.async_wait(new): deduplication/post-processing is still dispatched to the background, but the request waits for deduplication to finish before responding, soscan_addednotifications and the returned statistics reflect the deduplicated state. JIRA push, product grading and other non-deduplication tasks remain asynchronous and are not awaited.async(default, return immediately) andsync(run inline) round out the modes.block_executionflag, which is retained as the global "run all async tasks (notifications, JIRA, grading, ...) in the foreground" switch. A data migration seedsdeduplication_execution_mode='sync'for users who hadblock_execution=True, and the resolver falls back toblock_execution → sync, so import behavior is unchanged for them.post_process_findings_batchnow stores its result (ignore_result=False) soasync_waitcan join on the dispatched batch. The wait is bounded by the newDD_DEDUPLICATION_ASYNC_WAIT_TIMEOUTsetting (default 60s) so a missing or stuck worker degrades to responding anyway rather than hanging.deduplication_completeboolean to the import/reimport response indicating whether deduplication had finished by the time the response was produced (trueforsyncand forasync_waitwhen it completed within the timeout;falseforasync).scan_addednotifications deduplication-aware: once deduplication has completed, refresh the duplicate status of the affected findings from the database and split the new / reactivated / untouched lists into real and duplicate sub-lists, exclude duplicates from the headline count (so an all-duplicate import sendsscan_added_empty), and surface the duplicate groups in the mail and webhook templates. In plainasyncmode the lists are left untouched, preserving historical behavior.deduplication_execution_modefrom the user's profile (the API resolves it in the serializer), so UI imports honor the profile setting instead of always defaulting to async. (No per-import selector is added to the UI form in this change.)deduplication_execution_modein the user profile form and the user detail view, and add 2.60 upgrade notes.