UN-3719 [FIX] reaper poison orphan-claims — 404 (not 500) for a deleted execution → GC the claim

[muhammad-ali-e]

What

The reaper's orphan-claim sweep can now GC claims whose execution was deleted, instead of failing on them every cycle.

Why

pg_orchestration_claim has no FK to workflow_execution, so retention/cleanup that deletes old executions leaves orphaned claim rows. The sweep reads a claim's execution_id and looks it up (in workflow_execution) to decide GC-vs-recover — but the backend retrieve swallowed Http404 in a bare except Exception and returned 500. The reaper's _execution_status treats any failure as transient (raises → retain the claim), so a permanently-deleted execution poisoned every sweep:

RuntimeError: status read failed for execution 64d791a2… (refusing to mark ERROR on an unconfirmed status)
Reaper: orphan-claim sweep — 3 of 4 row(s) failed (left for the next sweep); 1 swept.

Observed live in integration: 3 claim rows (dated Apr–Oct) whose execution + file-execution rows are gone (0 rows), failing every 5-min sweep.

How

1. Backend retrieve(): catch Http404 / WorkflowExecution.DoesNotExist → 404 (before the generic 500).
2. Client plumbing: APIRequestError carries status_code; get_workflow_execution propagates it into ExecutionResponse.status_code.
3. Reaper: a confirmed 404 → _EXECUTION_GONE sentinel → GC the orphan claim (re-guarded on still-orphan-and-old). A 5xx / transport failure still raises → claim retained (unchanged — never terminalize on an unconfirmed read).

Safety

• The 404-GC only removes the queue-infra claim tombstone, never touches an execution/files, and re-guards on _orphan_claim_where (old + still-orphan) before deleting.
• Transient-vs-gone is a deterministic HTTP status, not string matching.
• Returning 404 for a missing execution is strictly more correct for every caller of the endpoint.

Tests

• Backend: retrieve returns 404 for a missing execution.
• Worker: _execution_status → GONE on 404, raises on 5xx/transport, returns status on success; _recover_one_claim GC's a gone execution and leaves it on a re-claim race.
• Worker 24 + backend 17 green.

UN-3719 — follow-up to #2172 / #2174 / #2175.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 62d81b88-4e06-4e9e-983e-6e40c42df069

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/UN-3445-reaper-orphan-claim-404

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR lets the reaper remove orphaned claims after their workflow execution has been deleted. The main changes are:

• Return a confirmed 404 when an execution is absent.
• Preserve HTTP status codes through the worker client response.
• Distinguish deleted executions from transient lookup failures.
• Clean up orphaned claims and barriers with race guards.
• Add backend and worker coverage for the new behavior.

Confidence Score: 5/5

This looks safe to merge.

• The updated backend check keeps scoped lookup failures separate from true deletion.
• The reaper requires both the 404 status and backend error marker before cleanup.
• Other lookup failures remain retryable, and cleanup retains its database race guards.

Important Files Changed

Filename	Overview
backend/workflow_manager/internal_views.py	Returns 404 only after an unscoped query confirms that the execution is absent.
workers/queue_backend/pg_queue/reaper.py	Handles confirmed deletion as a cleanup signal while retaining claims after unconfirmed failures.
workers/shared/clients/base_client.py	Adds HTTP status information to API request errors.
workers/shared/clients/execution_client.py	Propagates request status codes into failed execution responses.
workers/shared/data/response_models.py	Allows execution error responses to carry an optional HTTP status code.
backend/workflow_manager/execution/tests/test_pg_finalization_fixes.py	Covers missing and organization-scoped execution lookups.
workers/tests/test_reaper_orphan_claim_404.py	Covers deletion detection, transient failures, status propagation, cleanup, and re-claim races.

_{Reviews (2): Last reviewed commit: "UN-3719 [FIX] address review — gate GONE..." | Re-trigger Greptile}

[muhammad-ali-e]

Automated PR review (PR Review Toolkit: code-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer, comment-analyzer, code-simplifier). The status_code propagation chain (base_client → execution_client → ExecutionResponse → reaper) is correct and well-documented. Findings below are ranked; the org-scoped-404 issue already raised by @greptile-apps is not re-posted (comment 1 extends it with the infra/deploy-skew vector + a concrete fix). Highest-priority item is a second, un-updated caller of _execution_status.

[muhammad-ali-e]

[P1] Second caller of _execution_status doesn't handle _EXECUTION_GONE → misleading ValueError, barrier path never GC'd.

_execution_status now has two callers, but only _recover_one_claim (line 725) learned about the new sentinel. _recover_one_barrier (line 487) still guards only status is None and then calls ExecutionStatus.is_completed(status) (line 497). For a deleted execution that still has a stranded barrier row, status is _EXECUTION_GONE (a bare object()), so is_completed() does ExecutionStatus(<object>) → ValueError: Invalid status: <object …>. The per-row except Exception (line 588) catches it, rolls back, increments failed, and retains the row — so the barrier for a gone execution churns every sweep and can trip the "recovered nothing → systemic failure" escalation.

This isn't a fresh regression (pre-PR that path raised RuntimeError and also retained), but the fix is applied to the claim path and left uncovered on the sibling barrier path, and the failure now surfaces as a misleading exception rather than the deliberate one.

Root cause is the type: _execution_status(...) -> str | object | None (line 344) — object supersets str and None, so the union collapses to object and no checker flags the second call site. Fix both together:

1. Make the sentinel a closed type so call sites are exhaustively checkable, e.g. a single-member Enum (str | _StatusRead | None does not collapse). Note enum is not currently imported in this module.
2. Handle status is _EXECUTION_GONE in _recover_one_barrier before the is_completed call — delete the stranded barrier / pg_batch_dedup rows for the gone execution (mirror the claim-path GC), or at minimum skip cleanly with a clear log.

[muhammad-ali-e]

Good catch — fixed in 0a740136b. _recover_one_barrier now has a status is _EXECUTION_GONE branch before is_completed(), so the sentinel no longer hits ExecutionStatus(<object>) → ValueError. GONE → log + fall through to the existing cleanup (GC the orphaned pg_barrier_state + pg_batch_dedup, no status overwrite), symmetric with the claim path.

[muhammad-ali-e]

[P1] Extends @greptile-apps (org-scoped 404): a bare HTTP 404 is overloaded beyond "row deleted".

Greptile already flagged that an org-scope mismatch 404s a live execution here. The same status_code == 404-only check also fires for 404s that have nothing to do with the execution being gone: a rolling-deploy URL/API-version skew, a reverse proxy / load balancer, or an auth gateway. Any of these → _EXECUTION_GONE → _delete_orphan_claim. Blast radius is large: during a bad deploy every status read 404s, so every orphan claim is GC'd in a single sweep.

The fix data is already on the response: the backend returns a distinguishing body {"error": "WorkflowExecution not found"} (internal_views.py:163), and execution_client puts the 404 body into response.error (execution_client.py:140), which survives the to_dict() round-trip onto the object read here. Require both signals before acting:

if not getattr(response, "success", False):
    if (getattr(response, "status_code", None) == 404
            and "WorkflowExecution not found" in (getattr(response, "error", "") or "")):
        return _EXECUTION_GONE
    raise RuntimeError(...)  # any other 404 (route/proxy/auth) is transient → retain

A bare 404 with no app-level marker is exactly the "unconfirmed status" the docstring says must be retained. (Minor: HTTPStatus.NOT_FOUND reads better than the literal 404 and compares equal.)

[greptile-apps]

The concern is valid and the fix data is indeed already on the response. Tracing the chain confirms it:

1. Backend (internal_views.py) returns {"error": "WorkflowExecution not found"} with 404
2. base_client._make_request raises APIRequestError(f"Client error: 404 Not Found: {response_text}", status_code=404) — response_text is the JSON body string
3. execution_client.py catches the exception and sets error=str(e) → response.error = "Client error: 404 Not Found: {\"error\": \"WorkflowExecution not found\"}"

So "WorkflowExecution not found" in response.error will be True for the app-level 404 and False for a proxy/gateway/routing 404. The dual-check is the right fix:

Suggested change

	if getattr(response, "status_code", None) == 404:
	if not getattr(response, "success", False):
	if (
	getattr(response, "status_code", None) == 404
	and "WorkflowExecution not found"
	in (getattr(response, "error", "") or "")
	):
	return _EXECUTION_GONE

A proxy or load-balancer 404 would carry a generic HTML/plain-text body that won't contain the app-level marker, so it correctly falls through to the raise RuntimeError(...) path and retains the claim. The string is stable since it's set in a single place in internal_views.py — worth adding a brief comment there or in a constant if you want to make the coupling explicit, but the check itself is solid.

[muhammad-ali-e]

Adopted your dual-signal in 0a740136b: status_code==404 AND "WorkflowExecution not found" in (response.error or ""). A bare 404 (proxy/gateway/version skew) now raises → retained, so a bad deploy can't GC every claim in one sweep. Added tests: 404-with-marker→GONE, 404-without-marker→raise.

[muhammad-ali-e]

[P1/P2] retrieve emits an ambiguous 404 — root-cause backend fix for the org-scoped-404 issue.

Two problems make this 404 mean more than "row deleted", and the reaper turns any such 404 into an irreversible claim GC:

1. Org scoping. get_object() runs against the org-filtered queryset, and resolve_organization returns queryset.none() when the claim's organization_id doesn't resolve (or is stored in the wrong PK-vs-string format) → get_object_or_404 raises Http404 for an execution that still exists. Before returning 404, confirm the row is genuinely gone irrespective of org scope:

if not WorkflowExecution.objects.filter(id=kwargs.get('id')).exists():
    return Response({"error": "WorkflowExecution not found"}, status=404)
# else the row exists but is scoped out / another failure → 500 (retain), not 404

2. Over-broad try. This except wraps the entire method body, so a downstream Http404 (e.g. a nested get_object_or_404 in _get_source_config / _get_destination_config) is masked as "WorkflowExecution not found" → false 404 → GC of a live execution's claim. Narrow the try to just the get_object() call and run the context-building outside it, so downstream failures surface as 500 (retain).

Also: WorkflowExecution.DoesNotExist in the tuple is unreachable — DRF's get_object() raises Http404, never the bare model exception; drop it. And add organization_id to the log line so a wrongful GC is traceable.

[muhammad-ali-e]

Fixed in 0a740136b — took the unscoped-existence approach: on Http404/DoesNotExist, retrieve checks WorkflowExecution.objects.filter(id=…).exists() (the manager only scopes on user context, so this is unscoped in the service-to-service internal path). Genuinely absent → 404; exists-but-scoped-out or a nested downstream Http404 → 500 (retain). This closes both the org-scope and the over-broad-try holes at once. Backend test added for the scoped-out→500 case.

[muhammad-ali-e]

[P2] APIRequestError docstring overstates the status_code contract (rot risk).

The docstring presents a clean dichotomy — HTTP error response → carries a code; transport failure → None. But several branches raise APIRequestError without a status_code even though they are HTTP-level or request-level failures: rate-limit exhaustion (429, ~line 384), the catch-all "Other errors" branch (~line 396), and data-serialization failure (~line 325). So an HTTP error can also yield status_code=None.

This doesn't affect the reaper (404 is always populated on the client-error branch), but a future caller who trusts "any HTTP error carries a code" would be misled. Suggest narrowing the wording to: "carries the HTTP status for a non-timeout 4xx or a retry-exhausted 5xx (e.g. 404 / 500); None for transport failures and for a few paths that don't thread it (429-exhausted, serialization, generic fallback). Only rely on it for those 4xx/5xx cases."

(Related maintainability note, no change required: this reuses BaseResponse.status_code, which elsewhere defaults to 200/400 and means "this response's HTTP status" — here on the error path it means "the upstream call's status, or None." Same field, two null-semantics; worth a one-line note on the field, or a distinct name like upstream_status_code.)

[muhammad-ali-e]

Narrowed the docstring in 0a740136b to your wording: status_code is populated only for a non-timeout 4xx / retry-exhausted 5xx; None for transport failures and the 429-exhausted / serialization / generic-fallback paths — 'don't assume any HTTP error carries a code.'

[muhammad-ali-e]

[P2] The status_code propagation chain this feature depends on has no test coverage.

Every 404 test (test_reaper_orphan_claim_404.py) hand-sets .status_code on a MagicMock; none constructs a real ExecutionResponse, raises a real APIRequestError, or calls this method. So the three cooperating production pieces are untested end-to-end:

• base_client.py:345,366 — APIRequestError(..., status_code=response.status_code)
• this line — error_response(..., status_code=getattr(e, "status_code", None))
• response_models.py:220 — the new status_code param

A rename of APIRequestError.status_code, or a change to the getattr attribute name here, leaves every unit test green while _execution_status sees status_code=None on a real 404 → raises → the reaper is back to retrying deleted-execution claims forever (the exact bug this PR fixes). Add unit tests: (a) APIRequestError("m", status_code=404).status_code == 404 and default None; (b) patch the HTTP layer to raise APIRequestError(status_code=404/500) and assert the returned ExecutionResponse.status_code matches, and a plain Exception → None; (c) a base_client test that a 404 response raises APIRequestError whose .status_code == 404.

[muhammad-ali-e]

Added TestStatusCodePropagationChain in 0a740136b: constructs a real ExecutionAPIClient, patches the HTTP layer to raise a real APIRequestError(status_code=404/500), calls get_workflow_execution, and asserts the returned ExecutionResponse.status_code + .error marker. Plus TestApiRequestErrorStatusCode (carries 404 / defaults None). A rename in the chain now fails a test.

[muhammad-ali-e]

[P2] Add a non-404 4xx case to pin the "only 404 is GONE" boundary.

The suite covers 404 (GONE), 500 (raise), and transport/None (raise), but never a non-404 4xx such as 403/401/409. _execution_status special-cases only == 404; if someone later loosens that to >= 400 or in client_error_range, live claims sitting behind a transient 403 (auth blip) or 409 would be silently GC'd and no test would fail. Add e.g. test_403_raises_and_is_not_gone: _resp(False, status_code=403) must pytest.raises(RuntimeError) and must not return _EXECUTION_GONE.

[muhammad-ali-e]

Added test_403_raises_and_is_not_gone (and test_404_without_marker_raises) in 0a740136b — pins 'only the app-level 404 is GONE'; loosening to >=400 would now fail here instead of silently GC-ing live claims behind a 403/409.

[greptile-apps]

Confirmed on all findings. Full assessment below.

---

[P1] _recover_one_barrier — unhandled _EXECUTION_GONE → churning ValueError

This is the highest-priority gap. _recover_one_barrier (~line 487) calls _execution_status but only guards status is None; the _EXECUTION_GONE sentinel falls through to ExecutionStatus.is_completed(status), which attempts ExecutionStatus(<object>) → ValueError. The per-row except Exception catches it, increments failed, retains the row — so any barrier belonging to a deleted execution will raise a misleading ValueError on every sweep and can trip the "recovered nothing → systemic failure" escalation.

The fix is symmetric to the claim path: add a status is _EXECUTION_GONE branch before the is_completed call and GC the pg_barrier_state + pg_batch_dedup rows for the gone execution (or skip with a clear log if you prefer not to delete on the barrier path).

On the type annotation: -> str | object | None collapses to object and silences checkers on both call sites. A single-member Literal or Enum makes the union closed and lets a type checker flag any un-handled branch:

# e.g. a module-level Enum (requires import enum):
class _StatusSentinel(str, Enum):
    GONE = "EXECUTION_GONE"
_EXECUTION_GONE: Final = _StatusSentinel.GONE
# return type becomes: -> str | _StatusSentinel | None  (exhaustively checkable)

---

[P1] Scoped 404 + rolling-deploy skew → _EXECUTION_GONE on a live execution

Agree the status_code == 404 check is over-broad. The body-matching guard is the right fix — the distinguishing marker is already on the response:

if not getattr(response, "success", False):
    if (getattr(response, "status_code", None) == 404
            and "WorkflowExecution not found" in (getattr(response, "error", "") or "")):
        return _EXECUTION_GONE
    raise RuntimeError(
        f"status read failed for execution {execution_id} "
        f"(refusing to mark ERROR on an unconfirmed status)"
    )

A route-404 or proxy-404 during a rolling deploy has no app-level marker and will correctly retain. HTTPStatus.NOT_FOUND instead of the literal 404 is a worthwhile readability improvement.

---

[P1/P2] retrieve — org-scope ambiguity + over-broad try

Two changes needed in backend/workflow_manager/internal_views.py:

1. Global existence check before emitting 404 — prevents a live execution that org-scopes out from being treated as deleted:

except (Http404, WorkflowExecution.DoesNotExist):
    if not WorkflowExecution.objects.filter(pk=kwargs.get("pk") or kwargs.get("id")).exists():
        logger.info(...)
        return Response({"error": "WorkflowExecution not found"}, status=status.HTTP_404_NOT_FOUND)
    # Row exists but org-scoped out → let it fall through as 500 (retain)
    raise

2. Narrow the try so only get_object() is wrapped — downstream get_object_or_404 calls in context-building remain outside and surface as 500 (retain), not a false "not found" → GC.

Also: WorkflowExecution.DoesNotExist in the tuple is unreachable — DRF's get_object() always raises Http404, never the bare model exception. Drop it to avoid implying coverage that isn't there.

---

[P2] APIRequestError.status_code docstring overstates the contract

The "HTTP error → code; transport → None" dichotomy is incorrect: the 429-exhausted path, the catch-all "Other errors" branch, and serialization failures all raise APIRequestError without threading status_code. The current code is still safe for the reaper (only the 4xx client-error branch is relevant there), but the docstring will mislead future callers. Suggest narrowing to:

status_code is set for non-timeout 4xx and retry-exhausted 5xx responses; None for transport failures and several paths that don't thread it (429-exhausted, serialization, generic fallback). Only rely on it for 4xx/5xx HTTP responses from the internal API.

The field name collision with BaseResponse.status_code (which elsewhere defaults to 200/400 and means "this response's own status") is worth a one-liner clarifying it means "upstream call's HTTP status" here, or a rename to upstream_status_code if it won't break other consumers.

---

[P2] Propagation-chain integration test gap

All 404 tests in test_reaper_orphan_claim_404.py hand-set .status_code on MagicMock. A rename of APIRequestError.status_code or a change to the getattr key in execution_client.py leaves every test green while the live path regresses silently. Recommend adding:

• APIRequestError("msg", status_code=404).status_code == 404 and default None
• A get_workflow_execution test that raises a real APIRequestError(status_code=404) and asserts the returned ExecutionResponse.status_code == 404; plain Exception → None
• A base_client._make_request test that a 404 HTTP response raises APIRequestError with .status_code == 404

---

[P2] Missing non-404 4xx boundary test

Add test_403_raises_and_is_not_gone: _resp(False, status_code=403) must pytest.raises(RuntimeError) and must not return _EXECUTION_GONE. This pins the "only exact 404 + app-body is GONE" invariant and catches any future loosening of that boundary to >= 400.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[muhammad-ali-e]

Review addressed (0a740136b)

Thanks — several real P1s. All fixed:

[P1] Org-scoped / overloaded 404 → GC of a live claim (greptile + review). Two layers now guard it:

• Reaper _execution_status requires both status_code==404 and the backend marker "WorkflowExecution not found" in the body → a proxy / gateway / deploy-skew 404 (or org-scoped 404) can't map to GONE.
• Backend retrieve confirms the row is genuinely absent (unscoped .exists()) before 404; exists-but-scoped-out → 500 (reaper retains). This also closes the over-broad-try (a nested downstream Http404 for a different resource → 500, not a spurious "not found").

[P1] _recover_one_barrier didn't handle _EXECUTION_GONE → ExecutionStatus(<object>) ValueError churning every sweep. Added the sentinel branch (GC the orphaned barrier/dedup rows, no status overwrite), symmetric with the claim path.

[P2] APIRequestError docstring narrowed (429-exhausted / serialization / fallback don't thread status_code).

[P2] Propagation-chain coverage — new TestStatusCodePropagationChain exercises the real APIRequestError(status_code) → get_workflow_execution → ExecutionResponse.status_code/.error path (the mock-based tests couldn't catch a rename); plus TestApiRequestErrorStatusCode.

[P2] Boundary test — test_403_raises_and_is_not_gone + test_404_without_marker_raises pin "only the app-level 404 is GONE".

SonarCloud S5778 ×2 — one throwing call per pytest.raises (the api is built outside the with).

Worker 30 + backend 18 green.