fix: batch notification queries to eliminate N+1 cascading#1679
Open
ionfwsrijan wants to merge 13 commits into
Open
fix: batch notification queries to eliminate N+1 cascading#1679ionfwsrijan wants to merge 13 commits into
ionfwsrijan wants to merge 13 commits into
Conversation
Add _in_transaction flag to Database class so execute() only auto-commits when not inside an explicit transaction. Expand transaction scope in _upsert_findings_and_report and _upsert_findings_and_report_from_scanner to include the findings INSERT loop, ensuring all writes (findings, structured_json, reports, asset_services) succeed or roll back atomically. Previously, each write auto-committed independently, leaving orphaned findings, stale metadata, or permanently deleted asset services on crash. The DELETE-before-INSERT in replace_asset_services is now protected by the outer transaction. Fixes utksh1#1274
…mit inside transactions The execute() method was auto-committing after every query, which broke the transaction semantics when used inside async with db.transaction() blocks. The first execute() call would commit the BEGIN, and subsequent operations would run outside the transaction. - Add _in_transaction flag (initially False) - begin() sets _in_transaction = True - commit() and rollback() reset _in_transaction = False - execute() only auto-commits when _in_transaction is False This was described in the original commit message for efa59f0 but the database.py changes were not included.
…icate anyio_backend fixture - Revert validation.py hostname regex changes (out of PR scope) - Revert ci.yml backend-tests condition rewrite (out of PR scope, use original) - Remove duplicate anyio_backend fixture in conftest.py to fix event loop issues
replace_asset_services in platform_resources.py calls db.transaction(), but _upsert_findings_and_report and _upsert_findings_and_report_from_scanner already wrap the call in an outer transaction, causing the SQLite error 'cannot start a transaction within a transaction'. fix: transaction()/begin()/commit()/rollback() all check _in_transaction and become no-ops when already inside a transaction, allowing safe nesting.
…tandard_scanner This call was added as part of the PR scope but does not exist on main. ScannerBase._execute_command already handles this validation for modular scanners. For standard scanners, command args are plugin-provided module names (e.g. 'windows.pslist.PsList') that falsely trigger the hostname regex and cause 'Hostname did not resolve' errors, breaking the volatility integration test.
On main, _execute_standard_scanner does not accept safe_mode. This param was added as part of the scope-creep validation call which has now been removed. Restoring the original signature.
…rge conflict with PR utksh1#1314
ionfwsrijan
force-pushed
the
fix/1625-notification-n-plus-1
branch
from
9c0ac17 to
b074126
Compare
Contributor
Author
|
@utksh1 Please review this. The failing check is pre-exisiting |
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.
Description
The notification delivery pipeline performed a cascading N+1 query pattern: for each finding, it fetched all rules, and for each rule it checked delivery history and recorded it — resulting in 1 + N×(1 + 2M) DB queries. With 50 findings and 5 rules, this was 551 DB queries plus 250 outbound HTTP calls in a single synchronous chain.
Changes
backend/secuscan/notification_service.py:process_task_notifications()— Rewritten to batch all findings and all active rules upfront (2 queries). Evaluates (finding, rule) pairs in-memory. Dedup check uses a single batched query for all existing delivery history.backend/secuscan/notification_service.py:_deliver_rule_batch()— New helper that sends a single webhook per (rule, task) containing an array of findings instead of one webhook per (finding, rule). Reduces HTTP calls from N×M to M.backend/secuscan/executor.py:execute_task()— Notification dispatch is offloaded toasyncio.create_task()so slow webhook targets don't block task finalization.process_finding_notifications()anddeliver_via_rule()are preserved for backward compatibility.Impact
1 + N×(1 + 2M)to3 + M(findings + rules + dedup + M deliveries)event: finding.alert.batchwith afindingsarrayFiles Changed
backend/secuscan/notification_service.py— Major rewrite ofprocess_task_notifications, new_deliver_rule_batchhelperbackend/secuscan/executor.py— 1 line changed (async dispatch via create_task)Closes #1625