fix(worker): Don't fail Worker on Activity Task decode failures #1473

mjameswh · 2024-07-23T20:26:12Z

What was changed and why?

Catch all unexpected exceptions while decoding Activity Tasks in TS Worker. That notably includes things such as: protobuf fields that are unexpectedly null, errors while decoding heartbeat details, errors while instantiating activity interceptors, etc.
- These exceptions would previously result in failure of the Worker.
Also, include field names in error messages reported by tsToMs() and tsToDate() when the ts value is null or undefined.
- This should make it easier to triage and diagnose a specific category of issues we have been observing recently due to a change in the server-side protobuf library.

antlai-temporal

High level comments:
-Is this better for a "DEBUG" mode, or we want workers never to fail in production? In general, you want to fail fast when an unrecoverable error happens in prod, and not continue unless you know for sure the state has not been messed up... We are catching all exceptions, can we be sure of that?
-Are activities going to keep retrying, or just fail the workflow straight away (better the later).

Can other things apart from Date be unexpectedly null?

antlai-temporal · 2024-07-23T20:59:35Z

packages/worker/src/worker.ts

+                        nonRetryable: false,
+                      });


Do we want to retry? Parsing will fail again?

Payload conversion error could be due to a transient issue (e.g. an error accessing KMS, or old worker getting a task that would work in newer worker).

I agree that retrying might sometime be problematic, but it was already that way previously, and redefining that behavior is outside the scope of this PR.

antlai-temporal · 2024-07-23T21:10:27Z

packages/worker/src/worker.ts

                    break;
-                  }
-                  let args: unknown[];
-                  try {
-                    args = await decodeArrayFromPayloads(this.options.loadedDataConverter, task.start?.input);
-                  } catch (err) {
+                  } catch (e) {


Can we just move the break after the catch, and just have one break

That's arguable, but intentional from my part. I personally feel that duplicating the break statement here makes it more obvious that we reached the conclusion of these logic branches. The output = { ... } statements don't stand out enough by themselves.

antlai-temporal · 2024-07-23T21:12:49Z

packages/worker/src/worker.ts

-                  try {
-                    args = await decodeArrayFromPayloads(this.options.loadedDataConverter, task.start?.input);
-                  } catch (err) {
+                  } catch (e) {
+                    const error = ensureApplicationFailure(e);
                    output = {


What happens if it is not an application failure? Can you add a comment?

ensureApplicationFailure is an already existing internal helper function.

/** * If `error` is already an `ApplicationFailure`, returns `error`. * * Otherwise, converts `error` into an `ApplicationFailure` with: * * - `message`: `error.message` or `String(error)` * - `type`: `error.constructor.name` or `error.name` * - `stack`: `error.stack` or `''` */ export function ensureApplicationFailure(error: unknown): ApplicationFailure {

I don't think there's more to say than this. WDYT?

I thought that was an assertion, never mind...

antlai-temporal · 2024-07-23T21:15:59Z

packages/worker/src/worker.ts

+    throw ApplicationFailure.fromError(e, {
+      message: `Failed to parse heartbeat details for activity ${activityId}: ${errorMessage(e)}`,
+    });


Is this non-retriable?

the default is retryable.

Sorry, I meant whether we should make it non-retryable and fail the workflow

Payload conversion error could be due to transient issues.

bergundy · 2024-07-23T21:46:19Z

packages/worker/src/worker.ts

+    throw ApplicationFailure.fromError(e, {
+      message: `Failed to parse heartbeat details for activity ${activityId}: ${errorMessage(e)}`,
+    });
+  }
  return {


Can you make sure that if this code throws we log and error and dump the task we got so we can inspect this better?

I added log, including proto.toJSON() and the unaltered protbuf-encoded buffer (in base64 notation).

2024-07-24T04:45:57.463Z [ERROR] Error while processing ActivityTask.start: Expected scheduledTime to be a timestamp, got null { sdkComponent: 'worker', taskQueue: 'test', error: TypeError: Expected scheduledTime to be a timestamp, got null at requiredTsToMs (/Users/jwatkins/Development/Temporal/TypeScriptSDK/sdk-typescript-2/packages/common/src/time.ts:37:11) at Worker.extractActivityInfo (/Users/jwatkins/Development/Temporal/TypeScriptSDK/sdk-typescript-2/packages/worker/src/worker.ts:1064:43) at runNextTicks (node:internal/process/task_queues:60:5) at listOnTimeout (node:internal/timers:540:9) at process.processTimers (node:internal/timers:514:7) at <anonymous> (/Users/jwatkins/Development/Temporal/TypeScriptSDK/sdk-typescript-2/packages/worker/src/worker.ts:870:28), task: '{"taskToken":"ODQxYjEyM2EtYjc5MS00ZmE3LThmODctMTI3YjgxOWZlZWUw","start":{"workflowExecution":{"workflowId":"wfid","runId":"runId"},"activityType":"dummy","currentAttemptScheduledTime":{"seconds":"0","nanos":0},"startedTime":{"seconds":"0","nanos":0},"attempt":1,"scheduleToCloseTimeout":{"seconds":"60","nanos":0},"startToCloseTimeout":{"seconds":"60","nanos":0},"heartbeatTimeout":{"seconds":"60","nanos":0}}}', taskEncoded: 'CiQ4NDFiMTIzYS1iNzkxLTRmYTctOGY4Ny0xMjdiODE5ZmVlZTAaNhoNCgR3ZmlkEgVydW5JZCoFZHVtbXlSBAgAEABaBAgAEABgAWoECDwQAHIECDwQAHoECDwQAA==' }

bergundy · 2024-07-23T21:47:07Z

packages/worker/src/worker.ts

-                    args = await decodeArrayFromPayloads(this.options.loadedDataConverter, task.start?.input);
-                  } catch (err) {
+                  } catch (e) {
+                    const error = ensureApplicationFailure(e);


I'd want to ensure we have error logs in addition to failing the tasks. Didn't verify that this exists.

See my other comment.

mjameswh · 2024-07-24T05:10:37Z

Can other things apart from Date be unexpectedly null?

Of course. But for various reasons, timestamps appear more likely to pose issues than other data types in our specific use case.

…oralio#1473) (cherry picked from commit 4dd83c1)

… from #1473) (#1477)

fix(worker): Don't fail worker on Activity Task decode failures

b225b16

mjameswh requested a review from a team as a code owner July 23, 2024 20:26

mjameswh added 3 commits July 23, 2024 16:45

lint + remove unecessary checks

9bda82c

Fix failing tests

8d2882b

Fix minor build issue

53d39b0

antlai-temporal reviewed Jul 23, 2024

View reviewed changes

lint

d7d13de

bergundy reviewed Jul 23, 2024

View reviewed changes

Log more on fail to decode activity task

d97067d

bergundy approved these changes Jul 24, 2024

View reviewed changes

mjameswh merged commit 4dd83c1 into temporalio:releases/1.10.x Jul 24, 2024
69 checks passed

mjameswh deleted the safe-activity-worker branch July 24, 2024 17:03

mjameswh added a commit to mjameswh/sdk-typescript that referenced this pull request Jul 24, 2024

fix(worker): Don't fail Worker on Activity Task decode failures (temp…

8732c67

…oralio#1473) (cherry picked from commit 4dd83c1)

mjameswh mentioned this pull request Jul 24, 2024

fix(worker): Don't fail Worker on Activity Task decode failures (port from #1473) #1477

Merged

mjameswh added a commit that referenced this pull request Jul 25, 2024

fix(worker): Don't fail Worker on Activity Task decode failures (port…

cc6ee3b

… from #1473) (#1477)

mjameswh mentioned this pull request Jul 25, 2024

[Bug] Some TS Worker errors while processing a Task may cause complete Worker failure #1404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(worker): Don't fail Worker on Activity Task decode failures #1473

fix(worker): Don't fail Worker on Activity Task decode failures #1473

mjameswh commented Jul 23, 2024 •

edited

Loading

antlai-temporal left a comment

antlai-temporal Jul 23, 2024

mjameswh Jul 23, 2024

antlai-temporal Jul 23, 2024

mjameswh Jul 23, 2024

antlai-temporal Jul 23, 2024

mjameswh Jul 23, 2024 •

edited

Loading

antlai-temporal Jul 24, 2024

antlai-temporal Jul 23, 2024

bergundy Jul 23, 2024

antlai-temporal Jul 23, 2024

mjameswh Jul 23, 2024

bergundy Jul 23, 2024

mjameswh Jul 24, 2024

bergundy Jul 23, 2024

mjameswh Jul 24, 2024

mjameswh commented Jul 24, 2024

fix(worker): Don't fail Worker on Activity Task decode failures #1473

fix(worker): Don't fail Worker on Activity Task decode failures #1473

Conversation

mjameswh commented Jul 23, 2024 • edited Loading

What was changed and why?

antlai-temporal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjameswh Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjameswh commented Jul 24, 2024

mjameswh commented Jul 23, 2024 •

edited

Loading

mjameswh Jul 23, 2024 •

edited

Loading