Fix: non-map state creates lost runs every time#4626
Fix: non-map state creates lost runs every time#4626taylordowns2000 wants to merge 2 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
We should discuss the philosophy (perhaps not here?) In the 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 |
| id: dataclip_id, | ||
| project_id: project_id, | ||
| body: output_dataclip |> Jason.decode!(), | ||
| body: output_dataclip |> Jason.decode!() |> ensure_map(), |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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
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:
Closes #4627
Validation steps
red_team_test.exsfileAdditional 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
42isn't a valid final_state.(Queue up philosophical music... is
42a valid final state? No, it's not for me... but if we thought of the worker as independent from "OpenFn" then maybe42could 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!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)