Skip to content

fix(temporal): surface plugin failures for retries (supersedes #131) - #135

Open
trohitg wants to merge 7 commits into
mainfrom
fix/temporal-plugin-retry-boundary-followup
Open

fix(temporal): surface plugin failures for retries (supersedes #131)#135
trohitg wants to merge 7 commits into
mainfrom
fix/temporal-plugin-retry-boundary-followup

Conversation

@trohitg

@trohitg trohitg commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Supersedes #131 by @GammaFunds, whose commit fix(temporal): surface plugin failures for retries is the first commit on this branch with authorship preserved. It could not be extended in place: the PR branch predates main's rewrite of the same documentation and its fork refuses pushes that carry main's workflow-file changes, so this branch rebases the same change onto current main and adds the fix-ups the review on #131 asked for.

Problem

BaseNode.as_activity returned a plugin's structured {success: False, error_type, ...} envelope as a successful Temporal activity completion, so Temporal never saw a failure and no RetryPolicy applied on the deployed path. Every per-plugin retry_policy, the NON_RETRYABLE_ERROR_TYPES list, and the orchestrator's "after all retries" handling described behaviour that never ran; the in-process executor did retry, so canvas Run and deploy disagreed. The legacy execute_node_activity had the same gap, TemporalExecutor read a result key the workflow never returns (failed runs reported errors: []), and exceptions escaping the plugin (the payload-size guard) reached the executor with no error type at all.

What #131 got right and what it missed

The original change raises ApplicationError(error, type=error_type) at the boundary. Three things blocked it as-is:

  1. AgentWorkflow tool calls were scheduled with no retry policy, which is Temporal's unlimited default (docs.temporal.io/encyclopedia/retry-policies), so a raised tool failure would loop forever instead of reaching the handler that hands the error to the model.
  2. str(ActivityError) is "Activity task failed"; the plugin's text lives on the cause. MachinaWorkflow._wait_any_complete and the agent loop both stringified the wrapper, losing the message in errors[], the pause-on-failure reason, and the LLM tool message (which was also built with an unescaped f-string into a JSON literal).
  3. The test module registered stub nodes into the live registry (abstract=True was missing; a class-body _abstract = True is overwritten by __init_subclass__) and mocked temporalio.activity, so activity.info() was never exercised. Collected with tests/test_node_spec.py it failed twice.

And one unmanaged behaviour change: retries went live for all 148 registered node classes at once, including mutating nodes and deterministic failures.

Design (docs.temporal.io/develop/python/failure-detection, encyclopedia/retry-policies)

  • Classify at the source. services/plugin/retryability.py decides retryable from the real exception and BaseNode._wrap_error stamps it into the envelope: NodeUserError, validation, credential, invalid-parameters, output-contract and 4xx (except 408 / 425 / 429) are permanent; 5xx, timeouts, connection errors and unknown exceptions are transient; a boolean retryable attribute on the exception or its __cause__ wins, which is how a NodeUserError from the LLM unifier wrapping a rate-limited LLMError still retries. NodeExecutor classifies whatever escapes the plugin the same way, and _wrap_success now runs inside _execute_body's guard.
  • Typed failure at the boundary. services/temporal/_failures.py raises ApplicationError(type=error_type, non_retryable=..., details=[envelope]). Temporal stops retrying when either the type is in the scheduled non_retryable_error_types or the error was raised non_retryable, so a retryable failure carrying a non-retryable name is raised as <type>.retryable. The activity self-caps from activity.info() (min(scheduled policy, plugin effective policy)) with a pre-body refusal past the cap, so a re-dispatch after a crash or timeout never re-runs a one-attempt node, and histories already in flight with no policy are bounded too.
  • Attempts follow the node's annotations. BaseNode.effective_retry_policy(): a class-declared retry_policy wins; triggers and mutating nodes (destructive, readonly: False, or no annotations) get one attempt; read-only nodes keep three. annotations.readonly therefore means "safe to re-execute on a transient failure"; four misannotated nodes were corrected and tests/fixtures/effective_retry_attempts_snapshot.json pins the result per node type (36 nodes keep three attempts, 112 get one).
  • Consumers keep the plugin text. Both workflows unwrap the ActivityError cause with activity_failure_envelope; the model receives the failure envelope as JSON through _serialise_tool_result. The tool call and the taskManager preflight carry the tool plugin's policy, and MachinaWorkflow schedules the effective policy, all under workflow.patched("machina-plugin-failure-retries-v1") so pre-patch histories replay the exact command they recorded.
  • Broadcasts and context. Only the final attempt broadcasts error; earlier attempts stay executing with attempt / max_attempts / last_error (no new status string, no client change). NodeContext.attempt and NodeContext.idempotency_key (f"{workflow_run_id}-{activity_id}", per the Temporal Python docs) are available to plugins that opt into retries.
  • Rollback. TEMPORAL_PLUGIN_FAILURE_RETRIES=false (new Settings field, default on, evaluated activity-side) returns the envelope as a successful completion, the pre-fix behaviour.
  • SDK. temporalio floor raised to 1.31.0 (server/uv.lock is gitignored, so the floor is the tracked artifact).

Tests

  • tests/temporal/test_activity_failure_boundary.py rewritten on temporalio.testing.ActivityEnvironment with abstract=True stubs: success and ToolNode flat results unchanged, pre-executed and disabled passthrough, typed failures with details[0], the .retryable type suffix, retry_after to next_retry_delay, flag off, single terminal broadcast, executing-with-last_error on non-final attempts, infra exceptions re-raised identically, heartbeats, the self-cap matrix, the pre-body refusal, and extras forwarding.
  • tests/test_failure_classifier.py, tests/test_effective_retry_policy.py (with the registry snapshot), tests/temporal/test_machina_failure_unwrap.py (cause unwrapping, errors[], the pause reason end to end, patch-open and patch-closed policy resolution), tests/temporal/test_agent_tool_retry_policy.py (tool policy under the patch, the pre-patch command without one, the envelope the model sees), tests/temporal/test_executor_result_errors.py, plus extensions to test_dispatch.py (legacy activity), test_retry_policies.py, test_output_contract.py, test_plugin_contract.py.
  • The SDK replay gate (tests/temporal/test_agent_workflow_replay.py) gains a tool-call scenario that records the new retry policy into a real history and replays it.

Verification on the merged tree: full default backend suite 3522 passed, 2 skipped (the test_tikhub.py file, run separately, 74 passed); touched files clean under ruff --select E,F,W; git diff --check clean. No client files change.

Release notes

Retries are now live on the deployed path for read-only nodes; mutating nodes and triggers get one attempt unless they declare a policy. Expect more activity_retry WARN lines, since they now cover retryable plugin failures and not only worker crashes. A Reset is not required (the patch marker keeps pre-patch histories replayable) but is recommended for long-lived deployments so their tool calls pick up bounded retries. Rollback is TEMPORAL_PLUGIN_FAILURE_RETRIES=false.

🤖 Generated with Claude Code

GammaFunds and others added 7 commits September 7, 2026 23:59
The activity failure boundary relies on ApplicationError details,
activity.info().retry_policy, and temporalio.testing.ActivityEnvironment.
All three exist in 1.30, but the merged dependabot bump (#111) never
reached the lockfile because server/uv.lock is gitignored; pin the floor
so a fresh `uv sync` resolves the version the change was verified on.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…etryable lists

TEMPORAL_PLUGIN_FAILURE_RETRIES (default true) gates the new activity
failure contract from the activity side, the same posture as the
WORKFLOW_CONTROL_PAUSE_ON_FAILURE knobs, so flipping it never touches a
recorded workflow command. false restores the pre-fix behaviour where a
failure envelope is returned as a successful completion and never
retried.

OutputValidationError joins both non-retryable lists (an Output-contract
violation is a plugin bug; re-running re-bills the work and fails the
same way) and the plugin-side default also refuses Cancelled.
SINGLE_ATTEMPT_RETRY is the one-attempt policy effective_retry_policy
hands to mutating nodes and triggers.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
annotations.readonly is about to mean "safe to re-execute on a transient
failure" (BaseNode.effective_retry_policy). gmaps_create creates a
record, apify_actor starts a paid actor run, and vertex_cloud_tool
provisions resources, so none of them may claim readonly; emailRead only
reads mail and may keep three attempts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…errors

BaseNode.as_activity returned the structured {success: False} envelope as
a successful activity completion, so Temporal never saw a failure and no
RetryPolicy applied; the legacy execute_node_activity did the same.
Every per-plugin retry_policy was dead configuration on the deployed
path.

- services/plugin/retryability.py classifies at the source, where the
  exception object still exists: NodeUserError, validation, credential,
  invalid-parameters, output-contract and 4xx (except 408/425/429) are
  permanent; 5xx, timeouts, connection errors and unknown exceptions are
  transient; a boolean `retryable` attribute on the exception or its
  __cause__ wins (a NodeUserError from the LLM unifier wrapping a
  rate-limited LLMError still retries). _wrap_error stamps the verdict
  as `retryable`; NodeExecutor classifies whatever escapes the plugin,
  and _wrap_success now runs inside _execute_body's guard so the
  payload-size NodeUserError is classified instead of escaping untyped.
- services/temporal/_failures.py raises ApplicationError(type=error_type,
  non_retryable=<verdict or last attempt>, details=[envelope]). Temporal
  vetoes by type name before it reads non_retryable, so a retryable
  failure carrying a non-retryable name is raised as "<type>.retryable".
  The activity self-caps from activity.info(): min(scheduled policy,
  plugin effective policy), with a pre-body refusal past the cap so a
  re-dispatch after a crash never re-runs a one-attempt node.
- BaseNode.effective_retry_policy: a class-declared retry_policy wins;
  triggers and mutating nodes (destructive / readonly False / no
  annotations) get one attempt; readonly nodes keep three. A snapshot
  fixture pins the attempt count per registered node type.
- Only the final attempt broadcasts "error"; earlier attempts stay
  "executing" with attempt / max_attempts / last_error. NodeContext gains
  attempt and idempotency_key (f"{workflow_run_id}-{activity_id}").
- The observability interceptor logs the plugin error_type and the retry
  verdict, and demotes terminal user-correctable failures to INFO.

The boundary suite runs the real activity callable inside
temporalio.testing.ActivityEnvironment with abstract=True stubs, replacing
the first version's MagicMock patch of temporalio.activity and the stub
classes that registered themselves into the live node registry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t the errors list

str(ActivityError) is "Activity task failed"; the plugin's envelope rides
on the cause. MachinaWorkflow._wait_any_complete and the AgentWorkflow
tool-call handler now unwrap it with activity_failure_envelope, so
errors[], the pause-on-failure reason, and the LLM tool message keep the
plugin's text (the tool message also goes through _serialise_tool_result
instead of an unescaped f-string JSON literal).

The AgentWorkflow tool call and the taskManager preflight were scheduled
with no retry policy at all, which is Temporal's unlimited default; a
raised failure would have looped forever. Both now carry the tool
plugin's effective policy, and MachinaWorkflow schedules node activities
with effective_retry_policy, gated by machina-plugin-failure-retries-v1
so pre-patch histories replay the exact command they recorded.

TemporalExecutor read result["error"] while the workflow returns
errors; failed runs reported no errors at all.

The SDK replay gate gains a tool-call scenario that records the new
retry policy into a real history and replays it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cy, and idempotency keys

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants