perf(api): ingest compliance overviews in a single transaction#11875
perf(api): ingest compliance overviews in a single transaction#11875pedrooot wants to merge 4 commits into
Conversation
|
✅ All required changelog fragments are present. |
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks compliance requirement overview ingestion to run within a single per-scan database transaction using configurable batched ChangesCompliance requirement COPY/persist rework
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Job as create_compliance_requirements
participant Persist as _persist_compliance_requirement_rows
participant Copy as _copy_compliance_requirement_rows
participant DB as Database
participant ORM as ComplianceRequirementOverview
Job->>Persist: rows_factory
Persist->>Copy: tenant_id, scan_id, rows_factory(), batch_size
Copy->>DB: set tenant RLS config
Copy->>DB: delete existing rows (tenant_id, scan_id)
loop each batch
Copy->>DB: copy_expert(CSV buffer)
end
alt COPY succeeds
Copy->>DB: commit()
Copy-->>Persist: total rows committed
else COPY fails
Copy->>DB: rollback()
Persist->>ORM: filter(scan_id).delete()
Persist->>ORM: bulk_create(rows_factory())
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/backend/tasks/jobs/scan.py`:
- Around line 102-106: Validate the configured COPY batch size at startup in
scan.py where COMPLIANCE_COPY_BATCH_SIZE is read with env.int. Reject zero or
negative values before they reach the batching path, ideally by enforcing a
positive integer or raising a startup error during module initialization so
compliance ingest cannot begin with an invalid batch size.
- Around line 437-443: The admin COPY path in scan.py bypasses RLS, so the batch
rows must be validated before serialization in the loop that calls
_compliance_requirement_rows_to_csv and cursor.copy_expert. In the
batch-processing logic, reject or filter any row whose tenant_id or scan_id does
not exactly match the function arguments for the current scan, and only pass
validated rows into the COPY buffer. Make this check as close as possible to the
batched(rows, batch_size) processing so mixed-tenant data cannot be inserted
through the raw admin path.
- Around line 1825-1828: The scan rematerialization path in scan.py leaves stale
ComplianceRequirementOverview rows behind when compliance_template is empty
because _persist_compliance_requirement_rows is only reached inside the
non-empty template branch. Update the scan persistence flow around
requirements_created so the scan-scoped delete still runs even when there are no
template requirements, while preserving the existing transactional behavior in
_persist_compliance_requirement_rows and the _copy_compliance_requirement_rows
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0993c72c-22ba-451c-9656-4fefe659310e
📒 Files selected for processing (3)
api/CHANGELOG.mdapi/src/backend/tasks/jobs/scan.pyapi/src/backend/tasks/tests/test_scan.py
| # Rows per COPY statement when ingesting compliance requirement overviews. All | ||
| # batches of a scan share one transaction/commit; the batch size only bounds the | ||
| # client-side CSV buffer and how long each individual COPY statement runs on the | ||
| # writer (memory footprint, lock time and slow-statement logging under load). | ||
| COMPLIANCE_COPY_BATCH_SIZE = env.int("DJANGO_COMPLIANCE_COPY_BATCH_SIZE", default=2000) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the configured COPY batch size.
DJANGO_COMPLIANCE_COPY_BATCH_SIZE=0 or a negative value will flow into the batching path and can break every compliance ingest at runtime. Reject invalid values at startup.
Suggested fix
COMPLIANCE_COPY_BATCH_SIZE = env.int("DJANGO_COMPLIANCE_COPY_BATCH_SIZE", default=2000)
+if COMPLIANCE_COPY_BATCH_SIZE < 1:
+ raise ValueError("DJANGO_COMPLIANCE_COPY_BATCH_SIZE must be >= 1")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Rows per COPY statement when ingesting compliance requirement overviews. All | |
| # batches of a scan share one transaction/commit; the batch size only bounds the | |
| # client-side CSV buffer and how long each individual COPY statement runs on the | |
| # writer (memory footprint, lock time and slow-statement logging under load). | |
| COMPLIANCE_COPY_BATCH_SIZE = env.int("DJANGO_COMPLIANCE_COPY_BATCH_SIZE", default=2000) | |
| # Rows per COPY statement when ingesting compliance requirement overviews. All | |
| # batches of a scan share one transaction/commit; the batch size only bounds the | |
| # client-side CSV buffer and how long each individual COPY statement runs on the | |
| # writer (memory footprint, lock time and slow-statement logging under load). | |
| COMPLIANCE_COPY_BATCH_SIZE = env.int("DJANGO_COMPLIANCE_COPY_BATCH_SIZE", default=2000) | |
| if COMPLIANCE_COPY_BATCH_SIZE < 1: | |
| raise ValueError("DJANGO_COMPLIANCE_COPY_BATCH_SIZE must be >= 1") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/backend/tasks/jobs/scan.py` around lines 102 - 106, Validate the
configured COPY batch size at startup in scan.py where
COMPLIANCE_COPY_BATCH_SIZE is read with env.int. Reject zero or negative values
before they reach the batching path, ideally by enforcing a positive integer or
raising a startup error during module initialization so compliance ingest cannot
begin with an invalid batch size.
| for batch, _is_last in batched(rows, batch_size): | ||
| if not batch: | ||
| continue | ||
| batch_num += 1 | ||
| csv_buffer = _compliance_requirement_rows_to_csv(batch) | ||
| try: | ||
| cursor.copy_expert(copy_sql, csv_buffer) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate batch rows before admin COPY bypasses RLS.
The raw admin COPY path deletes by tenant_id/scan_id, but inserts whatever tenant_id and scan_id are present in each row. Since this bypasses normal RLS enforcement, reject any row that does not match the function arguments before serializing the batch.
As per coding guidelines, “Never query across multiple tenants or trust client-provided tenant_id without validation.” As per path instructions, this is a “multi-tenant Row-Level Security (RLS)” API.
Suggested fix
for batch, _is_last in batched(rows, batch_size):
if not batch:
continue
batch_num += 1
+ for row in batch:
+ if (
+ str(row.get("tenant_id")) != tenant_id
+ or str(row.get("scan_id")) != scan_id
+ ):
+ raise ValueError(
+ "Compliance COPY row tenant_id/scan_id mismatch"
+ )
csv_buffer = _compliance_requirement_rows_to_csv(batch)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for batch, _is_last in batched(rows, batch_size): | |
| if not batch: | |
| continue | |
| batch_num += 1 | |
| csv_buffer = _compliance_requirement_rows_to_csv(batch) | |
| try: | |
| cursor.copy_expert(copy_sql, csv_buffer) | |
| for batch, _is_last in batched(rows, batch_size): | |
| if not batch: | |
| continue | |
| batch_num += 1 | |
| for row in batch: | |
| if ( | |
| str(row.get("tenant_id")) != tenant_id | |
| or str(row.get("scan_id")) != scan_id | |
| ): | |
| raise ValueError( | |
| "Compliance COPY row tenant_id/scan_id mismatch" | |
| ) | |
| csv_buffer = _compliance_requirement_rows_to_csv(batch) | |
| try: | |
| cursor.copy_expert(copy_sql, csv_buffer) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/backend/tasks/jobs/scan.py` around lines 437 - 443, The admin COPY
path in scan.py bypasses RLS, so the batch rows must be validated before
serialization in the loop that calls _compliance_requirement_rows_to_csv and
cursor.copy_expert. In the batch-processing logic, reject or filter any row
whose tenant_id or scan_id does not exactly match the function arguments for the
current scan, and only pass validated rows into the COPY buffer. Make this check
as close as possible to the batched(rows, batch_size) processing so mixed-tenant
data cannot be inserted through the raw admin path.
Sources: Coding guidelines, Path instructions
| # The delete of the scan's previous rows happens inside the same | ||
| # transaction as the inserts (see _copy_compliance_requirement_rows). | ||
| requirements_created = _persist_compliance_requirement_rows( | ||
| tenant_id, _iter_compliance_requirement_rows() | ||
| tenant_id_str, scan_id_str, _iter_compliance_requirement_rows |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear stale rows when the compliance template is empty.
Persistence now owns the scan-scoped delete, but it is only called inside if compliance_template. If a scan is rematerialized with an empty template, old ComplianceRequirementOverview rows can survive while the result reports requirements_created == 0.
Suggested fix
requirements_created = _persist_compliance_requirement_rows(
tenant_id_str, scan_id_str, _iter_compliance_requirement_rows
)
+ else:
+ requirements_created = _persist_compliance_requirement_rows(
+ str(tenant_id), str(scan_instance.id), lambda: ()
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The delete of the scan's previous rows happens inside the same | |
| # transaction as the inserts (see _copy_compliance_requirement_rows). | |
| requirements_created = _persist_compliance_requirement_rows( | |
| tenant_id, _iter_compliance_requirement_rows() | |
| tenant_id_str, scan_id_str, _iter_compliance_requirement_rows | |
| # The delete of the scan's previous rows happens inside the same | |
| # transaction as the inserts (see _copy_compliance_requirement_rows). | |
| requirements_created = _persist_compliance_requirement_rows( | |
| tenant_id_str, scan_id_str, _iter_compliance_requirement_rows | |
| ) | |
| else: | |
| requirements_created = _persist_compliance_requirement_rows( | |
| str(tenant_id), str(scan_instance.id), lambda: () | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/backend/tasks/jobs/scan.py` around lines 1825 - 1828, The scan
rematerialization path in scan.py leaves stale ComplianceRequirementOverview
rows behind when compliance_template is empty because
_persist_compliance_requirement_rows is only reached inside the non-empty
template branch. Update the scan persistence flow around requirements_created so
the scan-scoped delete still runs even when there are no template requirements,
while preserving the existing transactional behavior in
_persist_compliance_requirement_rows and the _copy_compliance_requirement_rows
path.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11875 +/- ##
=======================================
Coverage 93.83% 93.83%
=======================================
Files 263 263
Lines 38692 38755 +63
=======================================
+ Hits 36305 36367 +62
- Misses 2387 2388 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Context
The compliance overview ingest (
create_compliance_requirements) wrote each scan as a standaloneDELETEtransaction followed by 10,000-rowCOPYbatches, each on a new admin connection with its own commit. Under midday scheduled-scan load this produced back-to-back 13–15sCOPYstatements, commit/fsync pile-ups, dead-row churn on retries (orphan recovery re-enqueuesscan-compliance-overviews, deleting already-committed rows and re-inserting everything), and slow-statement log flooding — the workload behind the recent Aurora writer failovers.Changes
DELETEand allCOPYbatches now run on a single admin connection with a single commit. A failed ingest rolls back entirely, so retries never find committed partial data to delete — removing the delete-then-reinsert dead-row churn.DJANGO_COMPLIANCE_COPY_BATCH_SIZEenv var (default 2000, previously hardcoded 10000). Batches share the transaction, so this only bounds per-statement duration, client CSV buffer memory, and slow-query logging — tunable at runtime without a deploy.COPYfailure the whole ingest falls back to one RLS transaction (delete +bulk_create), re-streaming rows from a fresh iterator (rows_factory).Steps to review
Please add a detailed description of how to review this PR.
Checklist
Community Checklist
SDK/CLI
UI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit
New Features
Bug Fixes
Chores