ENG-3835: Fix PR status coordination between manual tasks and async tasks#8275
Closed
JadeCara wants to merge 7 commits into
Closed
ENG-3835: Fix PR status coordination between manual tasks and async tasks#8275JadeCara wants to merge 7 commits into
JadeCara wants to merge 7 commits into
Conversation
…asks PR status was written independently by concurrent task nodes with no coordination, causing two bugs: - Forward: PR stays requires_input after manual task completes when async task blinds the watchdog - Inverse: PR stays in_processing when manual task needs input but async task causes watchdog to skip the entire PR Fixes: - Add derive_privacy_request_status() helper that derives correct status from aggregate task states (requires_input > pending_external > in_processing) - Watchdog now checks async_type per-task instead of per-PR, so active async tasks skip only themselves, not the entire PR - _has_async_tasks_awaiting_external_completion filters out completed async tasks - ManualTaskGraphTask uses derive helper instead of blind status write Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
…t gap - Import TERMINAL_PRIVACY_REQUEST_STATUSES from schemas instead of redefining - Remove dead _has_async_tasks_awaiting_external_completion function - Add denied to terminal status test parametrization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s-coordination # Conflicts: # src/fides/api/service/privacy_request/request_service.py
derive_privacy_request_status can't see the current task's awaiting_processing state (not flushed yet). Use priority-aware direct assignment for the "entering wait" case — only escalate, never downgrade. derive remains available for re-derivation after task completion. Also fix test mocks returning 3-tuples for the now-4-tuple _get_request_task_ids_in_progress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a manual task completes (submitted data returned), re-derive the PR status from remaining task states. This transitions the PR from requires_input to in_processing (or pending_external if Jira tasks remain) after all manual input is provided. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (91.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8275 +/- ##
==========================================
+ Coverage 85.08% 85.09% +0.01%
==========================================
Files 669 669
Lines 43560 43613 +53
Branches 5119 5132 +13
==========================================
+ Hits 37062 37114 +52
Misses 5393 5393
- Partials 1105 1106 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tatus guard Three bugs fixed in the derive function: 1. Timing: moved derive call from _set_submitted_data_or_raise (before log_end) to after log_end() in access/erasure/consent_request methods. The old placement queried while the completing task was still awaiting_processing, returning the wrong status. 2. Jira misclassification: both user-input and Jira manual tasks use collection="manual_data". Added ManualTask.task_type join to distinguish them — only privacy_request type returns requires_input, jira_ticket returns pending_external. 3. Status guard: replaced TERMINAL_PRIVACY_REQUEST_STATUSES blocklist with _DERIVABLE_STATUSES allowlist (in_processing, requires_input, pending_external). Prevents overwriting paused, awaiting_email_send, requires_manual_finalization, etc. Also hoisted _STATUS_PRIORITY to module-level constant and wrapped the derive call in try/except (task completion is already durable). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Moved PR to new dev branch |
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
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.
Ticket ENG-3835
Description Of Changes
Privacy request status was written independently by concurrent task nodes with no coordination. This caused two bugs:
requires_inputafter manual task completes when an async task is present — the watchdog was blinded by staleasync_typeon completed tasksin_processingwhen a manual task needs input but an async task exists — the watchdog skipped the entire PR, so users couldn't find or complete their manual tasksDesign principle: User-actionable statuses always surface. The PR status now answers "does someone need to do something?" —
requires_input>pending_external>in_processing. Only derivable statuses are ever modified; all others (terminal, paused, awaiting_email_send, etc.) pass through untouched.Code Changes
request_service.py:derive_privacy_request_status()— derives correct PR status from aggregate RequestTask state_DERIVABLE_STATUSESallowlist (in_processing, requires_input, pending_external) instead of terminal blocklist — prevents overwriting paused, awaiting_email_send, requires_manual_finalization, etc.ManualTask.task_typeto distinguish user-input tasks (requires_input) from Jira tasks (pending_external) — both sharecollection="manual_data"async_typeper-task instead of per-PR — active async tasks skip only themselves, not the entire PRasync_typeto_get_request_task_ids_in_progressyield_has_async_tasks_awaiting_external_completion(replaced by per-task check)manual_task_graph_task.py:derive_privacy_request_statuscall to afterlog_end()in access/erasure/consent_request — the old placement ran before the completing task was marked complete, causing self-defeating status derivation_update_privacy_request_status_after_completion()helper with try/except safety (task completion is already durable via log_end's separate session)requires_input > pending_external > in_processing) — prevents Jira tasks from overwriting manual task'srequires_input_STATUS_PRIORITYto module-level constanttest_derive_privacy_request_status.py: 19 tests covering the derive helper — user-input tasks, Jira tasks (with realistic ManualTask fixtures), mixed scenarios, async connectors, terminal status guard, non-derivable status guard (paused, awaiting_email_send, etc.)test_requeue_interrupted_tasks.py: 4 new tests for async task status filtering and per-task behaviorSteps to Confirm
awaiting_processing— PR status should showrequires_inputrequires_inputtoin_processingrequires_inputonce the manual task node executes)pending_external(notrequires_input)Automated tests:
Pre-Merge Checklist
CHANGELOG.mdupdated