Add post-import processing lifecycle to Finding (processing_status) (1/3)

[devGregA]

Part 1/3 of a visibility series: #15150 (this PR) · #15151 · #15152

⚠️ Merge-order note (interacts with #15152)

This PR and #15152 both update the pinned query-count baselines in unittests/test_importers_performance.py and unittests/test_tag_inheritance_perf.py, each measured against dev without the other. They are functionally independent and can merge in either order — but whichever merges second needs a rebase with re-measured baselines, because the deltas add up. Example: EXPECTED_ZAP_IMPORT_V2 is 287 on dev, 288 in each PR alone, and 289 once both are in (+1 for this PR's batch stamp, +1 for #15152's created-events batch). I'll push the combined-baseline rebase on whichever PR lands second.

---

Add post-import processing lifecycle to Finding (processing_status)

What

Findings now carry an explicit post-import processing state:

• processing_status: pending / processed / failed (TextChoices, mirroring the
  status-enum pattern used elsewhere in the platform)
• processed_at: when post-processing last completed

Importers create findings as pending; post_process_findings_batch (dedupe → false-positive
history → issue updater → grading → JIRA push) stamps processed on completion or failed on
error — the exception still propagates, but the failure is now visible on the findings
themselves instead of dying silently in a worker. Findings created outside the import pipeline
(manual entry, API) default to processed and never enter pending.

Why

Support keeps fielding variants of "the import ran but nothing happened / findings are stuck /
close-old didn't fire" where post-processing died invisibly (worker OOM, task crash) and nothing
in the product showed it. This is the smallest primitive that makes pipeline state observable,
and it defines the boundary of "post-processing" as one terminating chain — groundwork for
surfacing pending/failed counts on import pages and health alerts for stuck findings.

Performance notes

• Column add is metadata-only on PostgreSQL 11+ (constant default, no rewrite, no backfill);
  existing rows correctly read processed.
• Stamping is a single bulk UPDATE per batch (same 1,000-finding batches the importer
  already uses) — no signals, no per-row saves.
• The index is partial (WHERE processing_status = 'pending'): only the small, hot pending
  working set is indexed; steady-state instances pay near-zero index maintenance.
• Matched findings on reimport are re-stamped on batch completion but never re-enter pending
  (no dashboard flicker mid-scan).
• Finding is pghistory-tracked, so each batch stamp produces one history row per finding via the
  existing triggers (same write class as dedupe's own updates). The migration regenerates the
  history triggers to include the new columns — migration auto-generated by makemigrations.

API

• processing_status and processed_at appear on the Finding API (read-only).
• New filter: /api/v2/findings/?processing_status=pending|processed|failed.

Tests

unittests/test_processing_status.py:

• imported findings end processed with processed_at set
• reimport re-stamps matched findings and processes new ones
• a crashing post-processing batch stamps failed (and still raises)
• manually created findings default to processed

🤖 Generated with Claude Code

[Maffooch]

Reviewed & tested — approving (milestone 3.1.100).

Validated against a running instance:

• Checked out the branch; migration 0278_finding_processing_status applies cleanly on top of bugfix (0277).
• Full unittests.test_processing_status suite passes (4/4).

Also confirmed via review: the helper.py change is a behavior-preserving try/finally wrap (status is stamped PROCESSED on success, FAILED on exception, then re-raised); the migration is metadata-only (constant default + nullable timestamp, safe on a populated table); and the per-batch cost is a single bulk UPDATE (+1 query/batch, no N+1). Self-contained first slice of the series.

[Maffooch]

Really like the direction here — making post-processing state observable is overdue. One correctness gap in the failed semantics, plus a follow-on suggestion.

failed won't actually fire on a JIRA push failure. In post_process_findings_batch, the JIRA step is jira_services.push(object_to_push) inside the try, but add_jira_issue/update_jira_issue catch every exception internally and return a (success, message) tuple rather than raising (see dojo/jira/helper.py:905 / :1070 — hard failures go through failure_to_add_message → return False, message). So:

• No exception propagates → the try/except never trips → the finally stamps processed even when the ticket failed to create.
• This holds in every execution mode, including the synchronous block_execution=True / deduplication_execution_mode=sync path — it's not just an async-dispatch artifact. In the default async mode the push is also a separate fire-and-forget task, so push() returns an AsyncResult the batch couldn't inspect even if it wanted to.

Net: processing_status = processed currently means "the batch reached the JIRA-push step," not "the ticket was pushed." Worth tightening since the PR explicitly lists JIRA push as part of the terminating chain.

Requested change (per-finding, keep JIRA async): have the push task (push_finding_to_jira / push_finding_group_to_jira) record its own outcome — when it returns False, stamp that finding (or each finding in the group) processing_status = failed. That keeps the async throughput/retry isolation while making the failed state truthful. Two things to handle:

• Ordering: the batch finally stamps processed after dispatching the async push, so the task must be the last writer. Fine in normal async execution (task runs later), but watch the eager/force_sync path where the push runs inline before the finally — there the finally would overwrite failed back to processed. Might need the batch to skip re-stamping findings the push already marked, or have the push write last unconditionally.
• Groups: object_to_push can be a Finding_Group; fan the status back out to its member findings.

Follow-on suggestion — capture the why, not just the that. Since the push already has the message string from (False, message), consider adding a nullable processing_error (TextField) alongside processing_status, populated on failed. Today that reason only lands in log_jira_alert; surfacing it on the finding makes the failed working-set self-service to triage and pairs well with the "pending/failed counts on import pages" groundwork you mention. Happy to spin this out as a separate issue if you'd rather keep this PR tight.

[valentijnscholten]

This was on my list to implement as well :-) The idea I had is that the "processing complete" would/could/should be set after all post processing is complete, not only deduplication (using the new option from #15007)
For example pushing to JIRA can take a while to complete. The dedupe itself is so fast nowadays that in a non-overloaded instance the time difference between finding creating and processed_at is going to be very small. In earlier conversations we considered multiple flags, or a json field with multiple flags. i.e. deduplication_status, jira_delivery_status (or external issue status, or github status, ...)
An easy escape here to make my comment not blocking the pr/feature would be to rename procesing_statuts to deduplication_status (and the timestamp field in a similar way)

[devGregA]

@Maffooch Thank you for the precise diagnosis — you were right on all three counts (the internal (success, message) swallow, the eager-path ordering hazard, and groups). Implemented as requested, plus the processing_error suggestion since it makes failed actionable rather than just visible:

Push task as writer of record — record_finding_push_outcome() in dojo/jira/helper.py, called from push_finding_to_jira and push_finding_group_to_jira:

• (False, message) → stamps the finding(s) failed with the helper's message in the new processing_error field.
• Group pushes fan the outcome out to every member finding; the group task's per-finding "separate ticket" updates are also stamped individually by their own results (they could previously fail invisibly too).
• A later successful push heals failed → processed and clears the error. Successful pushes never touch pending findings — the batch owns the happy-path stamp.

Ordering — handled exactly as you suggested: the batch's final bulk UPDATE now excludes failed rows, so:

• eager/force_sync: the inline push stamps failed inside the try; the finally no longer overwrites it back to processed (covered by test_batch_stamp_does_not_overwrite_inline_push_failure, which runs the real dispatch path under CELERY_TASK_ALWAYS_EAGER).
• async: the task runs after the batch stamp and is simply the last writer.

processing_error — nullable-in-spirit (blank=True, default="", editable=False → read-only in the API via the serializer's Meta.exclude convention), populated by both the push outcome and the batch's own exception path (the batch now captures str(e) before re-raising). Migration 0279 regenerates the pghistory triggers.

One deliberate semantic worth flagging: a finding stamped failed by a push stays failed through later non-pushing batches (the exclude clause) until a push actually succeeds — the failure is unresolved until then, so the field keeps telling the truth rather than being wiped by an unrelated reimport. If you'd rather have any successful batch clear it, that's a one-line change to the exclude.

7 new tests in unittests/test_processing_status.py (failure stamping + reason, heal, pending-untouched, group fan-out, eager-ordering, batch-reason capture). Full suite + perf baselines rerun locally.

Coordination note: this adds migration 0279_remove_finding_insert_insert_and_more (the auto-name from the pghistory trigger regen), which collides numerically with #15152's 0279_finding_lifecycle_event — since #15152 merges after this anyway, I'll renumber its migration to 0280 and rebase it (including re-verifying the combined perf baselines) as soon as this lands, unless you'd rather handle it there.

[devGregA]

@valentijnscholten Great to hear it was on your list too — and your comment sharpened the naming question nicely. With Cody's requested change now implemented (the JIRA push tasks stamp their own per-finding outcome, including the failure reason in a new processing_error field), I'd argue processing_status has become accurate rather than optimistic: it now reflects the terminus of the whole post-processing chain — dedupe, FP history, grading dispatch, and the JIRA delivery outcome — not just the dedupe step. So I'd propose keeping the name rather than narrowing to deduplication_status, which would immediately under-describe what it tracks.

On the multi-flag / per-stage design (deduplication_status, jira_delivery_status, github_status, …): I think that's the right evolution if per-stage granularity becomes a real need, and nothing here blocks it — a single terminal status + error reason can later be joined by per-stage fields (or the JSON variant you mentioned) without migrating this one away. For now, the pair (processing_status, processing_error) answers the support-facing question ("did everything finish for this finding, and if not, why") with one field to filter on, which was the goal of this PR.

Re: #15007 interplay — the batch remains the join point for async_wait, and the JIRA push stays fire-and-forget by design (per Cody's review); its outcome lands on the finding when it completes, whichever order that happens in. The ordering is handled so the last truthful writer wins in both eager and async execution.