Skip to content

Commit 3ab30c8

Browse files
midigofrankstuartc
authored andcommitted
Stop session crash on malformed or stale workflow references
Harden the collaborative save path against two crash modes that killed the session GenServer and its channel: - Stale cron_cursor_job_id reference: a cron trigger could point at a job that no longer exists in the workflow. The trigger changeset lacked a foreign_key_constraint, so the FK violation raised Ecto.ConstraintError instead of a changeset error. Declare foreign_key_constraint and clear the reference client-side in removeJob. - Malformed UUIDs in workflow changesets: :binary_id fields are not format-checked by cast/3, so an unsubstituted import placeholder only raised Ecto.ChangeError on insert/update. Add a reusable validate_uuid/2 helper to Lightning.Validators and apply it in the Job, Edge and Trigger changesets. Plus changelog entries for the collaborative save crash fixes.
1 parent 466a50d commit 3ab30c8

10 files changed

Lines changed: 176 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,17 @@ and this project adheres to
6161
[#4810](https://github.com/OpenFn/lightning/issues/4810)
6262
- Workflow channel raises an exception when fetching trigger auth methods for an
6363
unpersisted trigger [#4819](https://github.com/OpenFn/lightning/issues/4819)
64+
- Collaborative session no longer crashes when saving a cron trigger whose
65+
`cron_cursor_job_id` references a job that no longer exists. The `Trigger`
66+
changeset now declares the foreign key constraint, and `removeJob` clears any
67+
cron cursor pointing at a deleted job, so the violation surfaces as a
68+
validation error instead of an `Ecto.ConstraintError`.
69+
[#4816](https://github.com/OpenFn/lightning/issues/4816)
70+
- Collaborative session no longer crashes when a workflow payload contains a
71+
malformed UUID (e.g. an unsubstituted template placeholder) for a job,
72+
trigger, or edge id. These ids are now validated in the changesets, so the bad
73+
value returns a changeset error instead of raising an `Ecto.ChangeError`
74+
during insert. [#4816](https://github.com/OpenFn/lightning/issues/4816)
6475

6576
## [2.16.6] - 2026-05-27
6677

assets/js/collaborative-editor/stores/createWorkflowStore.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -941,12 +941,24 @@ export const createWorkflowStore = () => {
941941
// Find all incoming edges (where this job is the target)
942942
const incomingEdgeIndices = getIncomingEdgeIndices(edges, id);
943943

944+
// Find triggers whose cron cursor points at the job being removed.
945+
const triggersArray = ydoc.getArray('triggers');
946+
const triggers = triggersArray.toArray() as Y.Map<unknown>[];
947+
const cursorTriggers = triggers.filter(
948+
trigger => trigger.get('cron_cursor_job_id') === id
949+
);
950+
944951
ydoc.transact(() => {
945952
// Delete incoming edges first (highest index to lowest)
946953
incomingEdgeIndices.forEach(edgeIndex => {
947954
edgesArray.delete(edgeIndex, 1);
948955
});
949956

957+
// Clear any cron cursor reference to the job being removed
958+
cursorTriggers.forEach(trigger => {
959+
trigger.set('cron_cursor_job_id', null);
960+
});
961+
950962
// Then delete the job
951963
jobsArray.delete(jobIndex, 1);
952964
});

assets/test/collaborative-editor/stores/createWorkflowStore.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,6 +1139,54 @@ describe('WorkflowStore - removeJob with edge cleanup', () => {
11391139
expect(snapshotAfter.jobs).toHaveLength(1);
11401140
expect(snapshotAfter.jobs?.[0]?.id).toBe('job-a');
11411141
});
1142+
1143+
test('clears cron_cursor_job_id on a cron trigger pointing at the removed job', () => {
1144+
// Setup: cron trigger whose cursor points at Job B (no edge between them,
1145+
// mirroring the unfiltered "Cron Input Source" dropdown).
1146+
const triggersArray = ydoc.getArray('triggers');
1147+
const triggerMap = new Y.Map();
1148+
triggerMap.set('id', 'trigger-1');
1149+
triggerMap.set('type', 'cron');
1150+
triggerMap.set('enabled', true);
1151+
triggerMap.set('cron_cursor_job_id', 'job-b');
1152+
triggersArray.push([triggerMap]);
1153+
1154+
store.addJob({ id: 'job-a', name: 'Job A', body: 'fn(state => state)' });
1155+
store.addJob({ id: 'job-b', name: 'Job B', body: 'fn(state => state)' });
1156+
1157+
// Verify setup
1158+
let snapshot = store.getSnapshot();
1159+
expect(snapshot.triggers?.[0]?.cron_cursor_job_id).toBe('job-b');
1160+
1161+
// Action: Delete the cursor job
1162+
store.removeJob('job-b');
1163+
1164+
// Assert: Job B gone and the trigger's cursor reference cleared
1165+
snapshot = store.getSnapshot();
1166+
expect(snapshot.jobs).toHaveLength(1);
1167+
expect(snapshot.jobs?.[0]?.id).toBe('job-a');
1168+
expect(snapshot.triggers?.[0]?.cron_cursor_job_id).toBeNull();
1169+
});
1170+
1171+
test('leaves cron_cursor_job_id untouched when a different job is removed', () => {
1172+
const triggersArray = ydoc.getArray('triggers');
1173+
const triggerMap = new Y.Map();
1174+
triggerMap.set('id', 'trigger-1');
1175+
triggerMap.set('type', 'cron');
1176+
triggerMap.set('enabled', true);
1177+
triggerMap.set('cron_cursor_job_id', 'job-a');
1178+
triggersArray.push([triggerMap]);
1179+
1180+
store.addJob({ id: 'job-a', name: 'Job A', body: 'fn(state => state)' });
1181+
store.addJob({ id: 'job-b', name: 'Job B', body: 'fn(state => state)' });
1182+
1183+
// Action: Delete a job that is NOT the cursor target
1184+
store.removeJob('job-b');
1185+
1186+
// Assert: cursor still points at the surviving job
1187+
const snapshot = store.getSnapshot();
1188+
expect(snapshot.triggers?.[0]?.cron_cursor_job_id).toBe('job-a');
1189+
});
11421190
});
11431191

11441192
describe('WorkflowStore - AI Workflow Apply Coordination', () => {

lib/lightning/utils/validators.ex

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,37 @@ defmodule Lightning.Validators do
8080
end
8181
end
8282

83+
@doc """
84+
Validates that the given field(s) contain a well-formed UUID.
85+
86+
`:binary_id` fields are not format-checked by `cast/3` — a malformed value
87+
(e.g. an unsubstituted import placeholder) passes casting and only raises
88+
`Ecto.ChangeError` when dumped on insert/update. This converts that into a
89+
changeset error instead.
90+
91+
Only runs when a non-nil change is present for the field, so optional
92+
foreign keys left unset are unaffected.
93+
94+
```
95+
changeset
96+
|> validate_uuid([:id, :workflow_id])
97+
```
98+
"""
99+
@spec validate_uuid(Ecto.Changeset.t(), atom() | [atom()]) ::
100+
Ecto.Changeset.t()
101+
def validate_uuid(changeset, fields) when is_list(fields) do
102+
Enum.reduce(fields, changeset, &validate_uuid(&2, &1))
103+
end
104+
105+
def validate_uuid(changeset, field) when is_atom(field) do
106+
validate_change(changeset, field, fn _, value ->
107+
case Ecto.UUID.cast(value) do
108+
{:ok, _} -> []
109+
:error -> [{field, "is not a valid UUID"}]
110+
end
111+
end)
112+
end
113+
83114
@doc """
84115
Validates a URL in a changeset field.
85116

lib/lightning/workflows/edge.ex

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,12 @@ defmodule Lightning.Workflows.Edge do
9191

9292
def validate(changeset) do
9393
changeset
94+
|> validate_uuid([
95+
:id,
96+
:source_job_id,
97+
:source_trigger_id,
98+
:target_job_id
99+
])
94100
|> assoc_constraint(:workflow)
95101
|> assoc_constraint(:source_trigger)
96102
|> assoc_constraint(:source_job)

lib/lightning/workflows/job.ex

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ defmodule Lightning.Workflows.Job do
9797
|> validate_required(:name, message: "job name can't be blank")
9898
|> validate_required(:body, message: "job body can't be blank")
9999
|> validate_required(:adaptor, message: "job adaptor can't be blank")
100+
|> Validators.validate_uuid([:id])
100101
|> Validators.validate_exclusive(
101102
[:project_credential_id, :keychain_credential_id],
102103
"cannot be set when the other credential type is also set"

lib/lightning/workflows/trigger.ex

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ defmodule Lightning.Workflows.Trigger do
1313
"""
1414
use Lightning.Schema
1515
import Ecto.Query
16+
import Lightning.Validators
1617

1718
alias Lightning.Workflows.Job
1819
alias Lightning.Workflows.Triggers.KafkaConfiguration
@@ -121,7 +122,11 @@ defmodule Lightning.Workflows.Trigger do
121122
|> validate_required([:type])
122123
|> assoc_constraint(:workflow)
123124
|> validate_by_type()
125+
|> validate_uuid([:id, :cron_cursor_job_id])
124126
|> unique_constraint(:id, name: "triggers_pkey")
127+
|> foreign_key_constraint(:cron_cursor_job_id,
128+
message: "the referenced cursor job does not exist"
129+
)
125130
end
126131

127132
defp validate_cron(changeset, _options \\ []) do

test/lightning/workflows/edge_test.exs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,19 @@ defmodule Lightning.Workflows.EdgeTest do
1414
assert changeset.valid?
1515
end
1616

17+
test "a malformed uuid field is a changeset error, not an Ecto.ChangeError on save" do
18+
changeset =
19+
Edge.changeset(%Edge{}, %{
20+
workflow_id: Ecto.UUID.generate(),
21+
source_job_id: "__ID_JOB_Envoyer-dans-DHIS2__",
22+
target_job_id: Ecto.UUID.generate(),
23+
condition_type: :on_job_success
24+
})
25+
26+
refute changeset.valid?
27+
assert changeset.errors[:source_job_id] == {"is not a valid UUID", []}
28+
end
29+
1730
test "edges must have a condition" do
1831
changeset =
1932
Edge.changeset(%Edge{}, %{

test/lightning/workflows/job_test.exs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,22 @@ defmodule Lightning.Workflows.JobTest do
1616
end
1717

1818
describe "changeset/2" do
19+
test "a malformed id is a changeset error, not an Ecto.ChangeError on save" do
20+
# An unsubstituted import placeholder reaching :id (a :binary_id field)
21+
# passes cast/3 and would only raise when dumped on insert. validate_uuid
22+
# surfaces it as a changeset error instead.
23+
changeset =
24+
Job.changeset(%Job{}, %{
25+
id: "__ID_JOB_Envoyer-dans-DHIS2__",
26+
name: "Test Job",
27+
body: "fn(state => state)",
28+
adaptor: "@openfn/language-common@latest"
29+
})
30+
31+
refute changeset.valid?
32+
assert changeset.errors[:id] == {"is not a valid UUID", []}
33+
end
34+
1935
test "accepts keychain_credential_id in changeset" do
2036
workflow = insert(:workflow)
2137

test/lightning/workflows/trigger_test.exs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,39 @@ defmodule Lightning.Workflows.TriggerTest do
320320
assert get_field(changeset, :cron_cursor_job_id) == nil
321321
end
322322

323+
test "a malformed cron_cursor_job_id is a changeset error, not an Ecto.ChangeError on save" do
324+
changeset =
325+
Trigger.changeset(%Trigger{}, %{
326+
type: :cron,
327+
cron_expression: "* * * * *",
328+
cron_cursor_job_id: "__ID_JOB_Envoyer-dans-DHIS2__"
329+
})
330+
331+
refute changeset.valid?
332+
333+
assert changeset.errors[:cron_cursor_job_id] ==
334+
{"is not a valid UUID", []}
335+
end
336+
337+
test "cron_cursor_job_id pointing at a non-existent job is a changeset error, not a raise" do
338+
workflow = insert(:workflow)
339+
340+
trigger =
341+
insert(:trigger,
342+
workflow: workflow,
343+
type: :cron,
344+
cron_expression: "* * * * *"
345+
)
346+
347+
changeset =
348+
Trigger.changeset(trigger, %{cron_cursor_job_id: Ecto.UUID.generate()})
349+
350+
assert {:error, changeset} = Lightning.Repo.update(changeset)
351+
352+
assert {"the referenced cursor job does not exist", _} =
353+
changeset.errors[:cron_cursor_job_id]
354+
end
355+
323356
test "allows webhook_reply to be set for webhook triggers" do
324357
changeset =
325358
Trigger.changeset(%Trigger{}, %{

0 commit comments

Comments
 (0)