[bug] Clarify and Enforce Terminal Task State Precedence During Races Between Canceled, Completed, and Failed Outcomes

[darrelmiller]

Background

PR #364 introduces logic to forcibly move a task into the Failed state if an exception occurs in background processing, even if other terminal transitions may be racing with it (e.g., the task might be Completed or Canceled nearly simultaneously).

This approach improves situations where tasks are stuck in Working due to handler crashes, but it raises questions about which terminal outcome "wins" if multiple terminal updates race.

Discussion Points / Concerns

• Race condition: There is a window where a background failure process may overwrite an already persisted terminal state (such as Completed or Canceled) with Failed. This is because the recovery code pre-reads the state outside the lock and there is no atomic check-and-set for terminal transitions.
• Intended final state precedence:
  • Should an execution failure in a background drain always win and mark the task Failed, even if the user or normal agent flow already produced Completed or Canceled?
  • Or should Completed/Canceled be preserved as the final state if they're saved first, treating background failures as only relevant if the task is still non-terminal?
• Example scenario:
  • A user cancels a seemingly hung task, triggering a Canceled transition, but background work later fails—should Failed overwrite Canceled, or should Canceled be considered the correct terminal state?
• Current behavior: The current logic means last terminal state wins. This could surprise users (e.g., UI shows Failed even though user intentionally canceled), and may not match the intended experience.
• Recommendation:
  • Decide on and document the desired precedence of terminal state transitions (Failed, Completed, Canceled, etc.).
  • Consider enforcing this precedence atomically (lock + re-check, or in task projection) to avoid accidental overwrites.
  • Update tests and documentation to clarify which transitions are allowed and which are not.
  • Consider passing appropriate cancellation tokens in all recovery paths as a related minor improvement.

Next Steps

• Team should agree on "terminal state precedence" policies to guide resolution and implementation.
• If user intent (explicit cancel) should always result in Canceled, implementation must guard against overwriting with Failed due to late background errors.
• If execution correctness should always win, document and surface this for users.

This issue is to track the above questions and drive consensus and a follow-up fix/clarification.

[chopmob-cloud]

The core question is which terminal state wins, and the cleanest answer that is also atomically enforceable is first persisted terminal state wins: once a task has reached any terminal state (Completed, Canceled, Failed, Rejected), a later actor should not transition it to a different terminal state.

Rationale:

• It matches user intent in your example: an explicit Cancel, or a genuine Completion, should not be silently overwritten by a late background Failed.
• It is the only rule enforceable without cross-actor coordination, because "which came first" is decided by the store under the per-task lock at the moment of the write.

The implementation point is that the precedence has to be an atomic check-and-set under the per-task lock, and it has to cover every forced terminal writer, not just the background-failure path. Two writers currently share the same shape of race (state read and terminal check outside the lock, then apply under the lock with no re-check):

1. Background failure: TryTransitionToFailedAsync reads the state (current main line 565) and checks IsTerminal() (566) before ApplyEventAsync takes the lock. A Completed or Canceled that persists in that window is overwritten by Failed.
2. Cancel: CancelTaskAsync checks IsTerminal() (425) on a read taken before the lock, then applies the Canceled event under the lock (462) with no re-check. A Completed or Failed that persists in that window is overwritten by Canceled, so a task the agent actually completed can be reported as canceled.

The pattern to copy already exists in tree: SubscribeToTaskAsync reads and checks terminal state inside the AcquireTaskLockAsync block (lines 485 to 490). Generalising that to the two write paths above closes both races.

I have deterministic regression tests for both directions: a gated task store drives the interleaving so the race reproduces on every run rather than probabilistically. Both fail on current main and pass once the terminal re-check moves under the lock, on net8.0 and net10.0, with the full unit suite green. Happy to open a PR with the atomic guard plus the two tests once the precedence is settled, or align it to execution-correctness-wins instead if the team prefers that.