ENG-2526: Reprocess DSRs via /resubmit instead of /retry#8199
ENG-2526: Reprocess DSRs via /resubmit instead of /retry#8199nreyes-dev wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
nreyes-dev
added
the
high-risk
nreyes-dev
changed the title
|
/code-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: ENG-2526 — Reprocess DSRs via /resubmit
Overall this is a clean, well-structured change. The intent is clear, the changelog is thorough, and the core service logic (resubmit_privacy_request) handles the delete-and-recreate pattern correctly — preserving the original ID, re-approving if needed, and clearing stale execution state. The frontend wiring is a straightforward rename with no logic changes.
Key concerns
1. Missing status guard in bulk endpoint (needs discussion)
bulk_resubmit_privacy_requests only pre-validates deleted_at before calling the service. The service itself only blocks complete status. This means in_processing, pending, paused, and requires_input requests can all be bulk-resubmitted — deleting their ExecutionLog, RequestTask, and AuditLog rows while a Celery worker may still be holding a live reference to the request. bulk_restart_privacy_request_from_failure explicitly gates on error status to prevent exactly this kind of race. If broad status coverage is intentional (e.g. to handle stuck in_processing requests), it should be documented and the race condition considered. See inline comment at line 2174.
2. Double DB fetch per request (minor performance)
The batch pre-fetch is only used for the deleted_at guard; the service then fetches each row a second time. Same pattern as bulk_retry, so it's consistent, but noting it in case bulk operations grow large. See inline comment at line 2155.
3. Stale hook exports in the slice
useBulkRetryMutation and useRetryMutation are still exported but have no UI consumers after this change. Minor, but could cause confusion about which hooks to use going forward.
4. Test coverage gaps
test_bulk_resubmit_completed_request_rejected doesn't assert the failure message, and there's no test for in_processing status — the highest-risk case.
What looks good
- Service-level logic is solid and matches the single-request endpoint's pattern.
- Backwards-compatible
PrivacyRequestBulkSelectioninput handling (plain list → schema) is tested. - Auth scope (
PRIVACY_REQUEST_CREATEvsPRIVACY_REQUEST_CALLBACK_RESUME) correctly reflects the elevated privilege needed for resubmit vs retry. - Frontend change is minimal and correct.
🔬 Codegraph: connected (50455 nodes)
💡 Write /code-review in a comment to re-run this review.
| "data": {"privacy_request_id": privacy_request_id}, | ||
| } | ||
| ) | ||
| continue |
There was a problem hiding this comment.
src/fides/api/v1/endpoints/privacy_request_endpoints.py:2174
The only pre-call guard here is deleted_at. The service's resubmit_privacy_request only blocks complete status, so requests with in_processing, pending, paused, or requires_input status will proceed through. This is a meaningful difference from bulk_restart_privacy_request_from_failure, which explicitly requires error status before continuing.
Resubmitting an in_processing request deletes its ExecutionLog, RequestTask, and AuditLog rows while a Celery worker may still hold a reference to that privacy request. That could cause a silent worker failure or orphaned state.
Consider adding an explicit status guard (analogous to the one in bulk_restart_privacy_request_from_failure) or documenting why broader status coverage is intentional here. At minimum, blocking in_processing would prevent the most dangerous race.
if privacy_request.status not in (PrivacyRequestStatus.error, PrivacyRequestStatus.paused, ...):
failed.append(...)
continue| try: | ||
| resubmitted = privacy_request_service.resubmit_privacy_request( | ||
| privacy_request_id | ||
| ) |
There was a problem hiding this comment.
src/fides/api/v1/endpoints/privacy_request_endpoints.py:2155
The batch pre-fetch (lines 2155–2160) loads each PrivacyRequest to check deleted_at, but privacy_request_service.resubmit_privacy_request(privacy_request_id) then calls self.get_privacy_request(privacy_request_id) internally — a second DB round-trip per request. The pre-fetched object is otherwise unused by the service.
This is the same pattern as bulk_restart_privacy_request_from_failure so it's at least consistent, but it does mean every resubmit in a batch causes two sequential selects on the same row. If this endpoint is expected to handle large batches, passing the pre-fetched object into the service (or doing the deleted-at guard inside the service) would halve the DB load.
| useBulkApproveRequestMutation, | ||
| useBulkDenyRequestMutation, | ||
| useBulkResubmitMutation, | ||
| useBulkRetryMutation, |
There was a problem hiding this comment.
clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts:664
useBulkRetryMutation (line 664) and useRetryMutation (line 675) are still exported here, but after this PR no component imports them — their call sites in ReprocessButton.tsx and useReprocessPrivacyRequest.ts have been switched to the new resubmit hooks. The underlying /bulk/retry and /{id}/retry backend endpoints still exist, so keeping these exports isn't wrong, but it may be worth a comment noting they're intentionally retained for future use, or removing them now to avoid confusion about which hooks are canonical.
| body = response.json() | ||
| assert body["succeeded"] == [] | ||
| assert len(body["failed"]) == 1 | ||
| assert body["failed"][0]["data"]["privacy_request_id"] == privacy_requests[0].id |
There was a problem hiding this comment.
tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.py:9993
The assertion here only checks len(body["failed"]) == 1 and the privacy_request_id field, but not the failure message. Since the complete rejection path is handled entirely by the service (which raises FidesopsException), tightening this to also assert the failure message would confirm the right code path fired:
assert body["failed"][0]["message"] == "Cannot resubmit a complete privacy request"Also worth considering: there is no test for resubmitting a request in in_processing status (the most dangerous case given the execution-log deletion). Adding one — even to document that it currently succeeds — would pin the behavior and make any future status-guard additions obvious.
1 participant
Ticket ENG-2526
Description Of Changes
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works