DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Status #4837

pattisdr · 2024-04-26T22:57:01Z

Closes #PROD-2005

Description Of Changes

Specifically, if manual request approval is required and the privacy request fails, the Privacy Request can get stuck in an approved state. While I tested the "manual request approval" flag in development, the large majority of the "error" testing was done with no approval required. Good find by Roger!

Code Changes

Update the existing polling task that checks for Privacy Requests whose sub-tasks have all had a chance to run, but some are errored, and transitions the Privacy Request to "Error" to also look for the "Approved" status.
👉 Privacy Requests that are in-flight are expected to be in a "In-Processing" or "Approved" status, and I was only covering the first case.

Steps to Confirm

Setup DSR 3.0 and require manual request approval. The worker doesn't specifically need to be running here, you don't need parallelization to hit this case.

[execution]
require_manual_request_approval = true
use_dsr_3_0 = true

nox -s dev -- shell postgres
cd fides/clients
turbo run dev
Add a postgres integration in the frontend with a bad secret.
Submit a Privacy Request through the Privacy Center.
Previously, this would hang indefinitely in Approved. Now, this will hang briefly, but at the next poll, (check your terminal), you should see something like:

“Polling for privacy requests awaiting errored status change”
Marking access step of pri_f1433833-31ed-4d6f-b5d9-e415f1e18b1a as error

This will mark the Privacy Request as errored.
Fix the secret
Reprocess the privacy request
It should complete

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation (not needed)
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

…oth IN_PROCESSING and APPROVED privacy requests to see if they have failing Request Tasks. If a Privacy Request needed manual approval failed during processing, it could hang in the "Approved" state, because the polling process wasn't expecting this status. If a Privacy Request is hanging out in a Pending status it is likely awaiting approval. Privacy Requests that don't need approval are queued immediately and are transitioned to in-processing. If a Privacy Request is approved, it is queued and not transitioned to in-processing. Both of these states can indicate a Privacy Request that is in-flight.

…s sporadically on DSR 3.0 - I just don't think this is a good test.

vercel · 2024-04-26T22:57:07Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Apr 29, 2024 2:32pm

pattisdr · 2024-04-26T22:57:50Z

src/fides/api/service/privacy_request/request_service.py

+            .filter(
+                PrivacyRequest.status.in_(
+                    [PrivacyRequestStatus.in_processing, PrivacyRequestStatus.approved]
+                )
+            )


This is the fix: an important detail I missed is that a Privacy Request that is actively in-flight, can have a status of "In Processing" or "Approved", not just "In Processing"

This makes sense!

pattisdr · 2024-04-26T22:58:02Z

tests/conftest.py

@@ -35,6 +31,8 @@
 from fides.api.main import app
 from fides.api.models.privacy_request import (
    EXITED_EXECUTION_LOG_STATUSES,
+    generate_request_callback_pre_approval_jwe,
+    generate_request_callback_resume_jwe,


Just applying the results of nox -s static_checks

pattisdr · 2024-04-26T22:59:23Z

tests/ops/integration_tests/test_execution.py

+    @pytest.mark.usefixtures("use_dsr_2_0")
    @pytest.mark.asyncio
-    @pytest.mark.parametrize(
-        "dsr_version",
-        ["use_dsr_3_0", "use_dsr_2_0"],
-    )
    async def test_run_disabled_collections_in_progress(


I am removing running this flaky test on DSR 3.0. This test was written for DSR 2.0 but it fails occasionally on CI for DSR 3.0. It is hard to reproduce when you run unit tests locally, you have to be running Python 3.8.x, and it will maybe fail 1/20 of the time, although it fails more frequently in CI. I just don't think this is a good test here.

cypress · 2024-04-26T23:09:12Z

Passing run #7502 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `e989d40` into `5b4983d`...
Project: fides	Commit: `e6a796a56e ℹ️`
Status: Passed	Duration: 00:36 💡
Started: Apr 29, 2024 2:44 PM	Ended: Apr 29, 2024 2:45 PM

Review all test suite changes for PR #4837 ↗︎

…ametrization has been removed.

codecov · 2024-04-27T00:15:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (5b4983d) to head (e989d40).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4837   +/-   ##
=======================================
  Coverage   86.87%   86.87%           
=======================================
  Files         345      345           
  Lines       20856    20856           
  Branches     2727     2727           
=======================================
  Hits        18119    18119           
  Misses       2262     2262           
  Partials      475      475

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

galvana · 2024-04-29T15:55:12Z

Verified the privacy request status is set to Approved and that the job detects the errored request

2024-04-29 08:51:42 2024-04-29 15:51:42.991 | DEBUG    | fides.api.service.privacy_request.request_service:poll_for_exited_privacy_request_tasks:190 - Polling for privacy requests awaiting errored status change
2024-04-29 08:51:43 2024-04-29 15:51:43.021 | INFO     | fides.api.service.privacy_request.request_service:poll_for_exited_privacy_request_tasks:233 - Marking access step of pri_f32ab96e-05a7-420f-abe9-0cd745d84014 as error

Reprocessed successfully!

I also verified that the request goes to error if the integration isn't fixed but the request is reprocessed.

galvana

The change makes sense and I was able to verify this locally ✅

pattisdr · 2024-04-29T15:57:02Z

Thank you for your review @galvana ⭐

…us (#4837)

pattisdr added 3 commits April 26, 2024 17:48

Run nox - s static_checks.

190703c

Remove flaky DSR 3.0 test, this test was written for DSR 2.0, it fail…

5074143

…s sporadically on DSR 3.0 - I just don't think this is a good test.

pattisdr commented Apr 26, 2024

View reviewed changes

Update changelog

15d2944

pattisdr requested a review from galvana April 26, 2024 23:04

Remove dsr_version which is no longer a test fixture now that the par…

2f797ae

…ametrization has been removed.

Merge branch 'main' into PROD-2005_approved_privacy_requests_that_error

e989d40

galvana approved these changes Apr 29, 2024

View reviewed changes

pattisdr merged commit 29dd36a into main Apr 29, 2024
42 checks passed

pattisdr deleted the PROD-2005_approved_privacy_requests_that_error branch April 29, 2024 16:03

pattisdr added a commit that referenced this pull request Apr 29, 2024

DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Stat…

a925cd5

…us (#4837)

cypress bot mentioned this pull request Apr 29, 2024

Release 2.35.0 #4832

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Status #4837

DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Status #4837

pattisdr commented Apr 26, 2024 •

edited

Loading

vercel bot commented Apr 26, 2024 •

edited

Loading

pattisdr Apr 26, 2024 •

edited

Loading

galvana Apr 29, 2024

pattisdr Apr 26, 2024

pattisdr Apr 26, 2024 •

edited

Loading

cypress bot commented Apr 26, 2024 •

edited

Loading

codecov bot commented Apr 27, 2024 •

edited

Loading

galvana commented Apr 29, 2024

galvana left a comment

pattisdr commented Apr 29, 2024

DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Status #4837

DSR 3.0 Bugfix: Approved -> Failed Privacy Requests Don't Change Status #4837

Conversation

pattisdr commented Apr 26, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Apr 26, 2024 • edited Loading

pattisdr Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

galvana Apr 29, 2024

Choose a reason for hiding this comment

pattisdr Apr 26, 2024

Choose a reason for hiding this comment

pattisdr Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

cypress bot commented Apr 26, 2024 • edited Loading

Passing run #7502 ↗︎

Review all test suite changes for PR #4837 ↗︎

codecov bot commented Apr 27, 2024 • edited Loading

Codecov Report

galvana commented Apr 29, 2024

galvana left a comment

Choose a reason for hiding this comment

pattisdr commented Apr 29, 2024

pattisdr commented Apr 26, 2024 •

edited

Loading

vercel bot commented Apr 26, 2024 •

edited

Loading

pattisdr Apr 26, 2024 •

edited

Loading

pattisdr Apr 26, 2024 •

edited

Loading

cypress bot commented Apr 26, 2024 •

edited

Loading

codecov bot commented Apr 27, 2024 •

edited

Loading