Skip to content

Fix: non-map state creates lost runs every time#4626

Open
taylordowns2000 wants to merge 2 commits intomainfrom
return-42-for-lost
Open

Fix: non-map state creates lost runs every time#4626
taylordowns2000 wants to merge 2 commits intomainfrom
return-42-for-lost

Conversation

@taylordowns2000
Copy link
Copy Markdown
Member

@taylordowns2000 taylordowns2000 commented Apr 15, 2026

Description

This PR makes it so we don't automatically "lose" all runs when the user choses to return something other than a JSON object. To repro the bug, run this job code:

fn(state => {
  return 42;
})

Closes #4627

Validation steps

  1. Check out (and run!) the new red_team_test.exs file

Additional notes for the reviewer

@josephjclark , @stuartc , why do we let the worker successfully complete runs where the final state is an integer or a string? I'm OK wrapping it in "value" but a cleaner approach seems to be making sure it fails/crashes on the worker side since 42 isn't a valid final_state.

(Queue up philosophical music... is 42 a valid final state? No, it's not for me... but if we thought of the worker as independent from "OpenFn" then maybe 42 could be a totally fine input state, or output state.)

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our
Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review
    with Claude Code)
  • I have implemented and tested all related authorization policies.
    (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@github-project-automation github-project-automation bot moved this to New Issues in Core Apr 15, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.52%. Comparing base (920b8c2) to head (88c286f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4626      +/-   ##
==========================================
+ Coverage   89.51%   89.52%   +0.01%     
==========================================
  Files         444      444              
  Lines       21505    21510       +5     
==========================================
+ Hits        19250    19257       +7     
+ Misses       2255     2253       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephjclark
Copy link
Copy Markdown
Collaborator

We should discuss the philosophy (perhaps not here?)

In the runtime, 42 is a perfectly valid state. I won't take arguments on that - the runtime is just JSON in, JSON out.

We could be strict in the worker and ensure that an object is returned. I'd be happy implementing that instead (it would be a better fix) - so long as you can convince me of why?

Lightning has never really cared about output state before (that's always been weird to me but fine). It's starting to care now though, what with sync workflows and configurable cron. So who cares if a workflow returns 42 or true or an array? I expect 99.9% of use cases to return { count: 42 } or { success: true }, but even so I don't know why we need to have an opinion on that.

id: dataclip_id,
project_id: project_id,
body: output_dataclip |> Jason.decode!(),
body: output_dataclip |> Jason.decode!() |> ensure_map(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42 is valid JSON AFAIK. Why do we need to ensure a map? Like where does Lightning actually care? Isn't the value arbitrary?

@@ -0,0 +1,178 @@
defmodule Lightning.RedTeamTest do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never heard of this red team stuff until now, and I can't be the only one

If we're going to keep this file, can we just name this file for what it is, rather than giving it a code name? Sorry to be boring but sometime boring is how I feel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

Non-map final state leads to lost

2 participants