fix(#1137): add transaction context manager for atomic multi-write operations (duplicate, conflicts resolved)#1271
Conversation
…ite operations - Add Database.transaction() async context manager (begin/commit/rollback) - Wrap _upsert_findings_and_report in transaction (findings + report + resources) - Wrap _upsert_findings_and_report_from_scanner in transaction - Wrap cancel_task status update + audit log in transaction - Wrap replace_asset_services delete+insert in transaction - Wrap delete_task_records in transaction (replacing manual begin/commit/rollback) - Wrap upsert_vault_secret select-then-upsert in transaction - Keep existing execute() auto-commit behavior for backward compatibility
- Fix indentation in replace_asset_services for loop body after wrapping in async with db.transaction() - Run ruff format on all 4 changed files
…method, handler already in main.py
… preserve rate-limit headers in 429 handler
|
@utksh1 Please review this |
utksh1
left a comment
utksh1
left a comment
There was a problem hiding this comment.
Excellent work. This PR adds a proper transaction() async context manager (BEGIN/COMMIT/ROLLBACK) to the database layer and wraps the executor's previously non-atomic multi-write operations (task cancellation, findings+report upsert) so they now commit atomically — directly addressing #1137. I verified all 28 tests pass (test_bulk_delete_sqlite_limit + test_process_tree_cancellation).
Notably, this also fixes a critical bug currently on main: commit c5d1967 introduced os.environ.get() calls in config.py (scan_rate_limit/window/burst) without importing os, which breaks the entire backend import. This PR adds the missing import os. Confirmed this resolves the NameError: name 'os' is not defined that currently breaks all backend tests.
LGTM, merging.
|
@utksh1 Please add labels |
utksh1
added
level:intermediate
2 participants
Duplicate of #1233 with merge conflicts resolved and rebased onto latest main.
Problem
Every database write was auto-committed immediately (
execute()callsconnection.commit()after every query). Multi-write operations (findings insert +report insert, asset services delete+insert, task delete cascade) had no atomicity —
a crash midway would leave partial data.
Solution
Database.transaction()async context manager that wrapsBEGIN/COMMIT/ROLLBACK_upsert_findings_and_report(findings + report + result resources)_upsert_findings_and_report_from_scanner(same)cancel_task(status update + audit log)replace_asset_services(DELETE + INSERT loop)delete_task_records(replaced manualbegin()/commit()/rollback())upsert_vault_secret(SELECT-then-UPDATE/INSERT race)execute()retains its auto-commit default; callers that needatomicity opt in via
async with db.transaction():Files changed
backend/secuscan/database.py,backend/secuscan/executor.py,backend/secuscan/platform_resources.py,backend/secuscan/routes.pyCloses #1137