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

Is push out-of-date really an Error? #88

Closed
pbrisbin opened this issue Apr 1, 2020 · 4 comments
Closed

Is push out-of-date really an Error? #88

pbrisbin opened this issue Apr 1, 2020 · 4 comments
Labels

Comments

@pbrisbin
Copy link
Member

pbrisbin commented Apr 1, 2020

For a high-traffic PR, with a relatively slow Restyle process, we can end up in a case where multiple Restyle jobs kick off concurrently and later ones' push --force-with-lease errors as out of date.

I'm not sure what should happen here, it depends on what sort of ordering guarantees we we want to rely on...

We could consider this a no-op: assuming a later Restyler must've finished first, so our work can be safely discarded.

We could just push --force: assuming an earlier Restyler must've finished first, so our work is more accurate and should overwrite.

We could just keep things as they are: we don't know what order things happened in, so we might as well error instead of making any claim as to the validity of what we've done.

@DanielHabenicht
Copy link

Maybe block the execution of multiple jobs per branch?

@pbrisbin
Copy link
Member Author

pbrisbin commented Apr 1, 2020

Yeah, ideally we would serialize Jobs by PR (which is just repo+branch). Then we could do what Circle and Travis do with auto-cancelling redundant builds (anything where something newer is in-progress).

So maybe one initial pre-requisite is an ability to Cancel a running Job. Once we have that ability, we could talk about adding a pre-check for each PR event we process:

  • Is another Job for the same PR in-progress?
  • Is it newer or older than this commit?
  • If newer: "cancel" ourselves
  • If older: cancel it and run ourselves

@pbrisbin
Copy link
Member Author

As of restyled-io/restyled.io#203, our Jobs processor will look for concurrent Jobs and follow the cancel logic described above. When it chooses to cancel a running Job, it sends the container SIGQUIT. That signal is being ignored right now, but I'm using the associated log message to judge how often we may make use of this feature. If it seems compelling*, building support for SIGQUIT in Restyler would be the next step.

*Here, compelling means:

  1. We see it semi-frequently
  2. We see it in association with the push-out-of-date error (i.e. it would've prevented that error)

@pbrisbin
Copy link
Member Author

54d9c64 updates Restyler to exit (0) on SIGQUIT, tying the knot on this feature. Hope it does the right thing 🤞

@pbrisbin pbrisbin moved this to Done in Roadmap Mar 6, 2023
@pbrisbin pbrisbin added this to Roadmap Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

No branches or pull requests

2 participants