Skip to content

ENG-3835: Fix PR status coordination between manual tasks and async tasks#8275

Closed
JadeCara wants to merge 7 commits into
mainfrom
ENG-3835/fix-pr-status-coordination
Closed

ENG-3835: Fix PR status coordination between manual tasks and async tasks#8275
JadeCara wants to merge 7 commits into
mainfrom
ENG-3835/fix-pr-status-coordination

Conversation

@JadeCara
Copy link
Copy Markdown
Contributor

@JadeCara JadeCara commented May 22, 2026

Ticket ENG-3835

Description Of Changes

Privacy request status was written independently by concurrent task nodes with no coordination. This caused two bugs:

  1. Forward: PR stays requires_input after manual task completes when an async task is present — the watchdog was blinded by stale async_type on completed tasks
  2. Inverse: PR stays in_processing when 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 tasks

Design 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:
    • Added derive_privacy_request_status() — derives correct PR status from aggregate RequestTask state
    • Uses _DERIVABLE_STATUSES allowlist (in_processing, requires_input, pending_external) instead of terminal blocklist — prevents overwriting paused, awaiting_email_send, requires_manual_finalization, etc.
    • Joins to ManualTask.task_type to distinguish user-input tasks (requires_input) from Jira tasks (pending_external) — both share collection="manual_data"
    • Changed watchdog to check async_type per-task instead of per-PR — active async tasks skip only themselves, not the entire PR
    • Added async_type to _get_request_task_ids_in_progress yield
    • Removed _has_async_tasks_awaiting_external_completion (replaced by per-task check)
  • manual_task_graph_task.py:
    • Moved derive_privacy_request_status call to after log_end() in access/erasure/consent_request — the old placement ran before the completing task was marked complete, causing self-defeating status derivation
    • Added _update_privacy_request_status_after_completion() helper with try/except safety (task completion is already durable via log_end's separate session)
    • Added priority-aware status escalation (requires_input > pending_external > in_processing) — prevents Jira tasks from overwriting manual task's requires_input
    • Hoisted _STATUS_PRIORITY to module-level constant
  • test_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 behavior

Steps to Confirm

  1. Submit a DSR with both a manual task connection and an async (callback/polling) connection
  2. Wait for the manual task to enter awaiting_processing — PR status should show requires_input
  3. Complete the manual task input via admin UI — PR status should transition from requires_input to in_processing
  4. Verify that if the async task completes first, the manual task is still accessible (PR shows requires_input once the manual task node executes)
  5. Verify a Jira-only manual task shows pending_external (not requires_input)

Automated tests:

docker exec fides bash -c "uv sync && pytest --no-cov tests/fides/task/test_derive_privacy_request_status.py tests/fides/task/test_requeue_interrupted_tasks.py -v"

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • No UX review needed
  • Followup issues:
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

JadeCara and others added 2 commits May 22, 2026 14:24
…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>
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview May 26, 2026 11:48pm
fides-privacy-center Ignored Ignored May 26, 2026 11:48pm

Request Review

JadeCara and others added 2 commits May 22, 2026 15:08
…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
JadeCara and others added 2 commits May 22, 2026 16:15
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
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.09%. Comparing base (19e2311) to head (1e6d93d).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...rc/fides/api/task/manual/manual_task_graph_task.py 81.25% 2 Missing and 1 partial ⚠️

❌ 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…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>
@JadeCara
Copy link
Copy Markdown
Contributor Author

Moved PR to new dev branch

@JadeCara JadeCara closed this May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant