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

fix DeleteFolder action being sporadically issued by Podbox #1836

Closed
wants to merge 1 commit into from

Conversation

ichub
Copy link
Contributor

@ichub ichub commented Jul 23, 2024

fixes https://linear.app/0xparc-pcd/issue/0XP-1061/pretixpipelines-deletefolder-action-is-sporadic

determine whether to delete a pipeline's folder based on whether or not it has loaded at least once since server start, rather than based on whether the current instance of the class corresponding to the pipeline has loaded

…ot it has loaded at least once since server start, rather than based on whether the current instance of the class corrsponding to the pipeline has loaded
@ichub ichub marked this pull request as ready for review July 23, 2024 20:45
@ichub ichub requested a review from artwyman July 23, 2024 20:45
Copy link
Collaborator

@artwyman artwyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some thoughts, though I might be thinking in the wrong direction given my limited context on how the PODBox backend is structured. I'm glad to review this, though you might want to solicit a second review from another dev with more context too.

if (this.loaded) {
// If the Atom DB is not empty, i.e. if we have loaded atoms for this pipeline
// at least once since the server started, delete the folder.
if ((await this.db.load(this.id)).length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a cost (to fetch from the DB) so I have some thoughts on reducing duplication of that work:

  1. It seems like there's duplicate effort from loading the tickets above. Is it difficult to get this information from the same fetch. Maybe it's tricky since there could be no tickets for the user's email. At least if tickets is non-empty we can assume there are atoms without fetching again, right?
  2. It should at least be possible to do this only once per pipeline instance if the first time you do this fetch you set this.everLoaded.

There are some assumptions here, like the fact that the atom DB is never empty in an active/loaded system. What would happen if an admin simply deleted all the tickets? Would that mean no atoms, and no delete, so that the client-side cleanup would never happen? Or are atoms a more persistent concept than that?

@artwyman artwyman mentioned this pull request Jul 25, 2024
4 tasks
@robknight
Copy link
Member

I wonder if any of the problems occurred due to async requests occuring inbetween the this.db.clear() and this.db.save(), which are not atomic.

There's also a small possibility that a pipeline might serve stale atoms from the atom DB after it has been restarted, and maybe some mismatch between the new post-restart config and pre-restart atoms might cause a problem (I can't think of one but it did make me wonder).

@robknight
Copy link
Member

In #1830 I've solved the problem of concurrent reads occuring inbetween clear() and save() by serializing access to the atom DB using PQueue, with clearing and saving occuring atomically.

@ichub
Copy link
Contributor Author

ichub commented Aug 19, 2024

subsumed by #1839

@ichub ichub closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants