Skip to content

incorrect status after user cancels a job. #9281

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.

Sign up for GitHub

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

Jump to bottom
Closed
spikecurtis opened this issue Aug 23, 2023 · 3 comments
Closed

incorrect status after user cancels a job. #9281

spikecurtis opened this issue Aug 23, 2023 · 3 comments
Assignees
@spikecurtis
Labels
s2 Broken use cases or features (with a workaround). Only humans may set this. stale This issue is like stale bread.

Comments

@spikecurtis
Copy link
Contributor

I feel like our Status computation between Canceled and Failed is wrong

https://github.com/coder/coder/blob/31ffb566d0cedbf1d80f140bc9de36049dc1fe0a/coderd/database/db2sdk/db2sdk.go#L78C1-L85C39

We only return Canceled if there is no Error, but the terraform provisioner generally returns an Error when it is canceled, so the Status ends up Canceling -> Failed.

I don't think its a good idea for provisioners to not return an Error when canceled, because then we can't tell the difference between something that was genuinely canceled and a job that completed before the cancel was processed.

So, in practice, we never see the Canceled state.

I did some digging, and it was me that complained about not being able to get to the "failed" state after cancel in the first place: #1374

Perhaps we need to distinguish 3 cases in the response from the provisioner after you send the cancel.

  1. Job completed successfully before we processed the cancel
  2. Job canceled
  3. Job failed for a reason unrelated to the cancel
@spikecurtis spikecurtis added s2 Broken use cases or features (with a workaround). Only humans may set this. bug labels Aug 23, 2023
@spikecurtis spikecurtis self-assigned this Aug 23, 2023
@bpmct
Copy link
Member

bpmct commented Aug 23, 2023

I'm hesitant to turn canceled into 3 states. I propose we remove it entirely. I left some notes on the conditions I'd expect

Job completed successfully before we processed the cancel

I feel like we just show running in this case. I'm not convinced we necessarily need to explain to admins why this happens at this point

Job canceled

This is where I'd expect for the state to be canceled

Job failed for a reason unrelated to the cancel

If the user presses cancel, I'd expect the state to be canceled but I suppose we could settle for failed.

@spikecurtis spikecurtis changed the title in practice, we never see workspaces in Canceled state incorrect status after user cancels a job. Aug 24, 2023
@spikecurtis
Copy link
Contributor Author

Sorry, I don't mean that Canceled status should be split into 3 different statuses. What I mean is that when you click cancel, there are 3 different possible outcomes, which as you note @bpmct, are covered by the existing statuses.

The problem today, is that our code doesn't distinguish all 3 outcomes, and gives the wrong statuses.

If the Job completes successfully before we process the cancel, then today we show status Canceled.

If the Job errors (regardless of whether the error is due to the cancel or not) then today we show Failed.

@bpmct
Copy link
Member

bpmct commented Aug 25, 2023

Ahh I see.

@github-actions github-actions bot added the stale This issue is like stale bread. label Feb 22, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@spikecurtis spikecurtis

Labels
s2 Broken use cases or features (with a workaround). Only humans may set this. stale This issue is like stale bread.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@spikecurtis @bpmct