Merge pull request #472 from OpenFn/exit-reasons-2

Worker: only count leaf nodes when working out fail reasons

integration-tests/worker/CHANGELOG.md

@@ -1,5 +1,12 @@
 # @openfn/integration-tests-worker
 
+## 1.0.13
+
+### Patch Changes
+
+- Updated dependencies [7d350d9]
+  - @openfn/ws-worker@0.2.3
+
 ## 1.0.12
 
 ### Patch Changes

integration-tests/worker/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@openfn/integration-tests-worker",
   "private": true,
-  "version": "1.0.12",
+  "version": "1.0.13",
   "description": "Lightning WOrker integration tests",
   "author": "Open Function Group <[email protected]>",
   "license": "ISC",

packages/ws-worker/CHANGELOG.md

@@ -1,5 +1,11 @@
 # ws-worker
 
+## 0.2.3
+
+### Patch Changes
+
+- 7d350d9: Only consider leaf nodes when calculating attempt fail reasons
+
 ## 0.2.2
 
 ### Patch Changes

packages/ws-worker/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openfn/ws-worker",
-  "version": "0.2.2",
+  "version": "0.2.3",
   "description": "A Websocket Worker to connect Lightning to a Runtime Engine",
   "main": "dist/index.js",
   "type": "module",

packages/ws-worker/src/api/execute.ts

@@ -237,23 +237,27 @@ export function onJobComplete(
 
   delete state.activeRun;
   delete state.activeJob;
-  const { reason, error_message, error_type } = calculateJobExitReason(
-    job_id,
-    event.state,
-    error
-  );
-  state.reasons[job_id] = { reason, error_message, error_type };
-
-  return sendEvent<RUN_COMPLETE_PAYLOAD>(channel, RUN_COMPLETE, {
-    run_id,
-    job_id,
-    output_dataclip_id: dataclipId,
-    output_dataclip: stringify(event.state),
-
-    reason,
-    error_message,
-    error_type,
-  });
+  try {
+    const { reason, error_message, error_type } = calculateJobExitReason(
+      job_id,
+      event.state,
+      error
+    );
+    state.reasons[job_id] = { reason, error_message, error_type };
+
+    return sendEvent<RUN_COMPLETE_PAYLOAD>(channel, RUN_COMPLETE, {
+      run_id,
+      job_id,
+      output_dataclip_id: dataclipId,
+      output_dataclip: stringify(event.state),
+
+      reason,
+      error_message,
+      error_type,
+    });
+  } catch (e) {
+    console.log(e);
+  }
 }
 
 export function onWorkflowStart(

packages/ws-worker/src/api/reasons.ts

@@ -4,7 +4,7 @@ import type { ExitReason, ExitReasonStrings, State } from '../types';
 // This takes the result state and error from the job
 const calculateJobExitReason = (
   jobId: string,
-  state: State,
+  state: State = { data: {} }, // If somehow there is no state with the job, this function must not explode
   error?: any
 ): ExitReason => {
   let reason: ExitReasonStrings = 'success';
@@ -24,19 +24,23 @@ const calculateJobExitReason = (
 };
 
 const calculateAttemptExitReason = (state: AttemptState) => {
-  if (state.reasons) {
+  if (state.plan && state.reasons) {
     // A crash or greater will trigger an error, and the error
     // basically becomes the exit reason
+    // So If we get here, we basically just need to look to see if there's a fail on a leaf node
+    // (we ignore fails on non-leaf nodes)
+    const leafJobReasons = state.plan.jobs
+      .filter(({ next }) => !next || Object.keys(next).length == 0)
+      // TODO what if somehow there is no exit reason for a job?
+      // This implies some kind of exception error, no?
+      .map(({ id }) => state.reasons[id!]);
 
-    // So If we get here, we basically just need to look for the first fail
-    // otherwise we return ok
-    const fail = Object.values(state.reasons).find(
-      ({ reason }) => reason === 'fail'
-    );
-
-    return fail || { reason: 'success', error_type: null, error_message: null };
+    const fail = leafJobReasons.find((r) => r && r.reason === 'fail');
+    if (fail) {
+      return fail;
+    }
   }
-  // TODO what if somehow there are no runs?
+  return { reason: 'success', error_type: null, error_message: null };
 };
 
 export { calculateJobExitReason, calculateAttemptExitReason };

packages/ws-worker/test/api/reasons.test.ts

@@ -77,3 +77,28 @@ test('crash has priority over fail', (t) => {
 
   t.is(r.reason, 'crash');
 });
+
+// This means a job didn't return state, which isn't great
+// (and actually soon may be a fail)
+// But it should not stop us calculating a reason
+test('success if no state is passed', (t) => {
+  const jobId = 'a';
+  const state = undefined;
+  const error = undefined;
+  const r = calculateJobExitReason(jobId, state, error);
+
+  t.is(r.reason, 'success');
+  t.is(r.error_type, null);
+  t.is(r.error_message, null);
+});
+
+test('success if boolean state is passed', (t) => {
+  const jobId = 'a';
+  const state = true;
+  const error = undefined;
+  const r = calculateJobExitReason(jobId, state, error);
+
+  t.is(r.reason, 'success');
+  t.is(r.error_type, null);
+  t.is(r.error_message, null);
+});

packages/ws-worker/test/reasons.test.ts

@@ -58,6 +58,7 @@ const execute = async (plan) =>
 
 test('success', async (t) => {
   const plan = createPlan({
+    id: 'y',
     expression: '(s) => s',
   });
 
@@ -69,8 +70,9 @@ test('success', async (t) => {
 
 test('fail: error on state', async (t) => {
   const plan = createPlan({
+    id: 'x',
     expression:
-      'export default [(s) => ({ errors: { "job-1": { "message": "err", "type": "Error" } } })]',
+      'export default [(s) => ({ errors: { "x": { "message": "err", "type": "Error" } } })]',
   });
 
   const { reason } = await execute(plan);
@@ -81,6 +83,7 @@ test('fail: error on state', async (t) => {
 
 test('fail: type error', async (t) => {
   const plan = createPlan({
+    id: 'z',
     expression: 'export default [(s) => { s.data = s.data.err.y; return s; }]',
   });
 
@@ -95,6 +98,7 @@ test('fail: type error', async (t) => {
 
 test('fail: user error', async (t) => {
   const plan = createPlan({
+    id: 'w',
     expression: 'export default [(s) => { throw "abort!"; }]',
   });
 
@@ -128,6 +132,31 @@ test('fail: user error in the third job', async (t) => {
   t.is(reason.error_type, 'UserError');
 });
 
+// We should ignore fails on non-leaf nodes (because a downstream leaf may anticipate and correct the error)
+test('success: error in the second job, but ok in the third', async (t) => {
+  const plan = createPlan(
+    {
+      id: 'a',
+      expression: 'export default [(s) => s ]',
+      next: { b: true },
+    },
+    {
+      id: 'b',
+      expression: 'export default [(s) => {throw "abort!"}]',
+      next: { c: true },
+    },
+    {
+      id: 'c',
+      expression: 'export default [(s) => { s }]',
+    }
+  );
+
+  const { reason } = await execute(plan);
+  t.is(reason.reason, 'success');
+  t.is(reason.error_message, null);
+  t.is(reason.error_type, null);
+});
+
 test('crash: reference error', async (t) => {
   const plan = createPlan({
     expression: 'export default [() => s]',