This is going to require further review, but here is what I have so far.

Passing data on all active repos to each ManagedRepo instance seems wrong. It seems like there has to be a better way, though I appreciate it's a messy problem.

Overall, handling the tryDelete loop in its own thread seems fine.

However, it looks like some of the data access is not thread-safe. The checkQueue function is called both from rmtree in the delete thread and in main in the main thread. This function both reads and modifies self.delete_pids and self.delete_queue. We could easily get some odd behavior two calls overlap.

This could be resolved different ways -- restricting the checkQueue calls to only one thread or the other, or using locking.

I'll likely have more to say after a second look.

I can add the lock - it is simple. What I'm afraid of more is logic behind _find_expire_time. I've gut feeling that it doesn't work well for recreated older repos, but wasn't able to find any concrete case which could break it.

Active repos could be saved to RepoManager._active_repos and deleted after the loop ends, so it is not being copied around.

I would also like to add rmtree logic to deleteRepos thread but it is worth another PR later.

Regarding _find_expire_time(). I was worried about the case where a repo is manually triggered from an old event and kojira gives it an expire_ts based on some about-to-be-deleted repo from the same tag.

However, since we also check mtime in tryDelete, the age of such repos will end up being based on that instead.

So I think that part is ok, or at least as ok as it can be given the data we're working with.

Commit ee93da41 fixes this pull-request

Did you merge this without fixing the threading issue?

Because of the similarly named functions, I did not realize at first review that you'd removed queuing from tryDelete. Easy to confuse the rmtree function from koji.util with self.rmtree.

So, the good news is that allays my threading concerns, since self.rmtree and self.checkQueue are only called from the main thread.

However, I'm worried that serializing the deletes, even in a dedicated thread, could be a problem when there are maven repos involved. Deleting a normal repo is almost instantaneous, since it's only a few files that need to be unlinked. However, maven repos are large and complicated trees with lots and lots of files that need to be unlinked. This tends to be very slow, particularly over nfs.

Anyway, I think that we should look into making the actual deletes parallel again. We still have the delete queue in place. We'd just have to make sure to do it in a thread-safe way. It seems inconsistent to have the main thread queuing deletes in pruneLocalRepos, while the delete thread is running them directly.

I'll file a follow up for that. I don't think we need to hold 1.22 for it, though.

I've filed #2398 as a followup and marked it for 1.23