Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

History page crashes if job is removed from workflow after it's been run #1568

Closed
sentry-io bot opened this issue Dec 13, 2023 · 5 comments
Closed

History page crashes if job is removed from workflow after it's been run #1568

sentry-io bot opened this issue Dec 13, 2023 · 5 comments
Assignees
Labels
bug Newly identified bug unplanned Items included in a existing sprint that weren't part of the initial Sprint backlog

Comments

@sentry-io
Copy link

sentry-io bot commented Dec 13, 2023

To reproduce:

  1. Create a workflow with two steps.
  2. Run it.
  3. Delete the second step.
  4. Save it.
  5. Navigate to the history page.
  6. See crash.

Big question for @NickOpenFn and @stuartc - how do we expect this to behave? Should the job really be deleted? Should it be archived? What happens to previous attempts with runs for this job? (edit... see my comment below after a discussion with Elias.)

Sentry Issue: LIGHTNING-8B

KeyError: key :runs not found in: nil

If you are using the dot syntax, such as map.field, make sure the left-hand side of the dot is a map
  File "lib/lightning_web/live/run_live/workorder_component.ex", line 28, in LightningWeb.RunLive.WorkOrderComponent.set_work_order_details/2
  File "lib/lightning_web/live/run_live/workorder_component.ex", line 20, in LightningWeb.RunLive.WorkOrderComponent.update/2
  File "lib/phoenix_live_view/utils.ex", line 498, in Phoenix.LiveView.Utils.maybe_call_update!/3
  File "lib/phoenix_live_view/diff.ex", line 659, in anonymous fn/6 in Phoenix.LiveView.Diff.render_pending_components/6
  File "lib/enum.ex", line 2510, in Enum."-reduce/3-lists^foldl/2-0-"/3
...
(3 additional frame(s) were not displayed)

(KeyError key :runs not found in: nil

If you are using the dot syntax, such as map.field, make sure the left-hand side of the dot is a map)
@sentry-io sentry-io bot added the bug Newly identified bug label Dec 13, 2023
@elias-ba elias-ba self-assigned this Dec 13, 2023
@taylordowns2000
Copy link
Member

taylordowns2000 commented Dec 13, 2023

@elias-ba thanks for the quick call. notes here:

  1. you'll spike the "proper fix" (soft delete of job and edge, purged when associated activity is gone — same as credential.)
  2. soft delete feels right for EVERYTHING that is directly related to a workorder or an attempt. the history page is sacred, so you can request for things which build your history view to be deleted but we won't actually remove them until the associated history activity is purged based on your configured retention period. we've established this pattern already and we should continue to use it.)
  3. if the proper fix seems doable (and preferable) to blocking deletion altogether then we'll do it but we will not create any UI accomodation for viewing or restoring deleted jobs yet.
  4. the job will simply not appear on the canvas anymore; if the user views a run associated with that job, they will not be able to navigate to the job via the soon-to-come job inspector link; instead they'll see a "job has been deleted, contact superuser if you want to restore" indication

love your thinking here and eager to see the results of your spike for discussion tomorrow AM with @NickOpenFn and the guys

@taylordowns2000 taylordowns2000 added the unplanned Items included in a existing sprint that weren't part of the initial Sprint backlog label Dec 13, 2023
@NickOpenFn
Copy link

@taylordowns2000 Chatted to Elias (who also had a convo with @stuartc):

  1. We will use this issue to try recreate and fix the bug (no one can reproduce it at the moment).
  2. There is one code issue that is, in @elias-ba's words "Smelly" that he and @stuartc agree to resolve as part of this.
  3. The soft delete is a separate feature, and will be it's own card (and we all feel that there should perhaps be a bit of discussion as to whether this is the correct solution right now).

@NickOpenFn
Copy link

Issue for point 3 above #1570

@stuartc
Copy link
Member

stuartc commented Dec 14, 2023

@taylordowns2000 @NickOpenFn, I don't believe soft delete is necessary - it's another layer that avoids a larger set of problems.

I've had a "Snapshotting" feature in mind for about a year now. This would snapshot a workflow the first time it (in its current/edited form) is used on an Attempt. This would preserve the shape of the workflow enabling Attempts to reflect what they were run with when they actually were run and negate the need for soft delete on Jobs and Edges.

Unless theres something else that soft deleting would provide that wouldn't be addressed by snapshotting I'd strongly advise against it.

Re: #1570

@elias-ba
Copy link
Contributor

Closing this issue as the associated PR has been approved and merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Newly identified bug unplanned Items included in a existing sprint that weren't part of the initial Sprint backlog
Projects
None yet
Development

No branches or pull requests

4 participants