AIP-97 POC: does mark-failed belong in TaskFailureInfo? (respect the explicit user cancel)

[1fanwang]

Subsumed by #10 — the redesign folds mark-failed into failure_kind=USER (a plain enum value, no infra-named field on a non-infra thing). The "respect the explicit user cancel" outcome verified here holds in #10. Review #10 for the final shape.

---

AIP-97 POC — does mark-failed belong in TaskFailureInfo?

Open question: a user marking a task failed is a lifecycle action, not a failure cause. Should it
populate TaskFailureInfo (as source="user") or stay failure_details=None?

Option A (this branch): fold mark-failed into TaskFailureInfo

The Core API mark-failed path now passes TaskFailureInfo(source="user", infra_reason="manually_set_to_failed")
instead of None.

Live e2e (LocalExecutor + Postgres): a running task marked FAILED via the REST API. The listener
receives:

dag=mf_repro try=1 max_tries=1 failure_details=TaskFailureInfo(source='user', executor_kind=None, infra_reason='manually_set_to_failed', infra_metadata={})

Final state: state=failed try=1 max_tries=1 — the task terminated, it did not retry. Refund
lines: 0. So a user's explicit cancel is respected: the task dies as the user intended, it is not
refunded, and (once AIP-96 lands) a resumable operator must not resume it.

The load-bearing requirement: respect the explicit user cancel

A mark-failed is not a failure to recover from — it is the user saying "kill this." The design must
respect that: no refund, and no resume. The source is what carries that intent to the two gates:

• Refund gate: refunds only source == "infra", so a user cancel never gets an extra attempt.
• Resume gate (AIP-96): resumes only source == "infra", so a resumable operator's on_kill must
  not checkpoint-and-resume a user cancel.

Both gates already honor it — verified here for the refund (0 refund lines, terminal). The open
question is only how the cancel is signaled, not whether it is respected.

Pro of an explicit source: the "respect the user" check is direct and self-documenting
(source == "user"), rather than inferred from failure_details is None.

Con (a real smell this POC surfaced): the payload's other fields are infra-named —
infra_reason, infra_metadata, executor_kind. Folding a user cancel in forces
infra_reason="manually_set_to_failed", which reads wrong: a cancel has no infra reason. Every
system surveyed keeps a user lifecycle action separate from a failure cause — Temporal has distinct
CanceledFailureInfo / TerminatedFailureInfo types, not a "source" on its failure payload.

Option B (current, on #6/#7): mark-failed stays None

failure_details=None; the listener distinguishes a mark-failed via the error string
("...manually set to failed."). TaskFailureInfo stays reserved for actual failure causes
(app / infra / timeout), keeping the infra-named fields honest.

Live e2e (same deployment, refund flag ON), the bridge branch at the identical mark-failed:

RESULT: failed try=1 max_tries=1     (refund lines: 0)
dag=mf_repro try=1 max_tries=1 failure_details=None

Same terminal outcome as Option A — the cancel is respected (terminated at try=1, not refunded even
with refunds enabled). The only difference from Option A is the payload: None here vs
TaskFailureInfo(source='user', ...) there. Both gates key on source == "infra", so neither a
None nor a user cancel is ever refunded or resumed.

Pro: conceptually clean — the type never carries a non-failure. Con: a listener that wants to
treat mark-failed specially reads the error string, not a structured field.

Recommendation

The user's cancel must be respected either way — both gates key on source == "infra", so a user
cancel (Option A) and a None (Option B) are both left to terminate, never refunded, never resumed.
The choice is only how the cancel is signaled.

Lean toward a dedicated cancel/lifecycle signal rather than overloading either. Folding the
cancel into TaskFailureInfo(source="user") (Option A) forces infra_reason onto a thing with no
infra reason; leaving it None (Option B) makes the "respect the user" check an inference from
absence rather than an explicit intent. A distinct signal — mirroring Temporal's separate
CanceledFailureInfo / TerminatedFailureInfo — states the user's intent plainly and keeps
TaskFailureInfo reserved for real failure causes (app / infra / timeout). If a separate signal is
too much surface for now, Option B is the safer default: it never puts a non-failure into the
failure payload, and the "not infra" gate already respects the cancel.

Both options are safe for the retry budget: source="user" is never refunded, so mark-failed spends
the retry either way.

[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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c79aceb4-b4fe-488c-a040-2a7ac08391ed

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 aip97-poc-markfailed-source

---

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.}

[1fanwang]

Resolved in the current design: manual is a first-class failure_kind (MANUAL) and the refund fires only on INFRA, so a manual mark is never refunded. Closing; branch kept.