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

Consider also token expiration time for send submitted email #304

Closed
burnout87 opened this issue Dec 4, 2021 · 6 comments · May be fixed by #312
Closed

Consider also token expiration time for send submitted email #304

burnout87 opened this issue Dec 4, 2021 · 6 comments · May be fixed by #312
Assignees
Labels
enhancement New feature or request Epic
Milestone

Comments

@burnout87
Copy link
Collaborator

No description provided.

@volodymyrss
Copy link
Member

whenever the second "submitted" email is sent (after a delay), we need to give a bit more information about why this was needed.

@burnout87 burnout87 linked a pull request Dec 10, 2021 that will close this issue
@burnout87
Copy link
Collaborator Author

I am working on this point (#312): we want to consider also the token expiration time when deciding whether or not send another email with submitted status. Basically, we want to send this new email in case the elapsed time from the last email was greater than the intsub entry extracted from the token (or configuration), or more than the token expiration time.

Is it correct? Or am I missing something?

@volodymyrss
Copy link
Member

Maybe. I am not sure I do understand how you put it. intsub used for making the choice about submitting email should be intsub=max(token_expires_in, intsub_from_token)

@burnout87
Copy link
Collaborator Author

burnout87 commented Dec 17, 2021

intsub=max(token_expires_in, intsub_from_token)

This condition cannot actually work since once the token expires the request won't go through and fail before even reaching that point. I suggest to keep the same condition (with only intsub from the token). Of course still warn the user in case of multiple submitted emails are sent.

What about sending an email to the user, informing that the token is expired ? And this would be sent instead of the multiple submitted one.

@volodymyrss
Copy link
Member

volodymyrss commented Dec 17, 2021

intsub=max(token_expires_in, intsub_from_token)

This condition cannot actually work since once the token expires the request won't go through and fail before even reaching that point. I suggest to keep the same condition (with only intsub from the token). Of course still warn the user in case of multiple submitted emails are sent.

it can be a different request, with new token, for the same job_id.
Actually, the complexity we are addressing here (which is a convoluted complexity indeed) is precisely with repeating (at different time, different token) requests for the same job. The job may be actually resubmitted in such a case, for a variety of reasons. Should there be an email?

What if the job was last sent 1 minute ago? We had situations like this in the past. It was related to some dispatcher issues. This results in a storm of emails. They might still happen.
We should report them somewhere, to sentry (do we?).

1 year go?
Timeout is a simple and not unreasonable solution to distinguish this case.
If it becomes too unclear, after the grace period (defined from config/token) we could make a "resubmitted" email, indicating that it was submitted in the past. Maybe let's do that.
But not without blocking repeat submitted in short period - unless we are sure this does not happen anymore (we are not).

What about sending an email to the user, informing that the token is expired ? And this would be sent instead of the multiple submitted one.

We should not do anything when token is expired. That's the whole point of expiring tokens - they should be treated as invalid. Sending emails for invalid tokens is a security problem. It might be that expired token is compromised. Anyone finding any expired token anywhere would storm us/someone with emails. We have already left some tokens in the wild. Luckily they have expired. These tokens may not be used for any real action, especially sending emails, and should be discarded promptly.

We could however warn if the token is near-expired. It's tricky to know when. Like "your job is about to expire". Also regularly scanning existing directories might help. This it's done by dispatcher it's safe.

edit: let me express this in clear user stories and discuss before any further substantial action.

@volodymyrss
Copy link
Member

situation 1:

as happens currently

  • user creates token with lifetime 7 days on Monday
  • user intsub is 3 days
  • user submits a large job on Monday
  • On Thursday, user requests the job again. The status is still submitted, for one of the two reasons:
    • the job is still submitted - we do not currently have a "waiting" status. It's deduced by the dispatcher from the email history.
    • the job has been actually resubmitted/recreated on the backend. This is anomalous but may happen and there should be defined behavior in this case which does not confuse the user.
  • second "submitted" is sent.
  • User is confused

as should happen

  • within the expected job lifetime

situation 2:

  • intsub is set to one year
  • user submits a large job in January
  • the url to the job is inserted in the paper which is published
  • in May, user uses the url from the paper to get the products.
  • no "submitted" email is sent
  • user is confused why there is no email

summary suggestion

  • email should be sent if the job is still "live".
  • there are 3 sources of "liveness" now:
    • existing job scratch directory on dispatcher
    • still running job on the backend (possibly with expired token)
    • lifeness of the token
  • we should consider single guaranteed job liveness definition. The other ones might be larger, and if they do - we should have clear defined behavior
  • intsub effectively indicates user-desired extension of the job liveliness (i.e. user may want to reduce number of emails for newly submitted similar jobs). It's likely most user never need to set this. This was no really meant to be general-user-manipulated.
  • so the principal suggestion is to:
    • send second "submitted" email if the job token (not the user-provided token) is still valid
    • if the job is still submitted when it's token is expired - send us a sentry warning. Maybe add a short note to the user email indicating that "this job is taking longer than expected. We are looking into it"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Epic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants