Remove old timestamps in scheduleld transaction manager cleanup #571
Conversation
| let status = FlowTransactionScheduler.getStatus(id: id) | ||
| if status == nil || status == FlowTransactionScheduler.Status.Unknown { | ||
| transactionsToRemove.append(id) | ||
| } | ||
| transactionsToRemove.append(id) | ||
| } |
There was a problem hiding this comment.
by removing the conditional, I think we're appending adding ids that could be scheduled. is that right?
There was a problem hiding this comment.
or is that not an issue because we're getting timestamps before the current timestamp?
There was a problem hiding this comment.
It wouldn't remove scheduled ids because it is only doing timestamps in the past, but now that you mention it, I think it might remove ids that are supposed to execute in the same block but haven't actually been executed yet. I need to think about it a bit more, but I don't think that is an issue because this just removes the scheduled transaction resource and does not affect the handler
There was a problem hiding this comment.
I'll still add a check that the status can't be scheduled though just to be safe
There was a problem hiding this comment.
yeah, if anything likely more of an edge case where I cleanup something I want to cancel in the current or next block
| ids.remove(at: index!) | ||
| if ids.length == 0 { | ||
| self.idsByTimestamp.remove(key: timestamp) | ||
| self.sortedTimestamps.remove(timestamp: timestamp) |
There was a problem hiding this comment.
Since these were not removed prior, do we need some sort of backfill logic to remove irrelevant sorted timestamps?
There was a problem hiding this comment.
yeah, I'll add that
| // because some managers on mainnet have already hit the limit and we need to batch them | ||
| // to make sure they get cleaned up properly | ||
| // This will be removed eventually | ||
| if transactionsToRemove.length > 100 { |
There was a problem hiding this comment.
It's an edge case, but if one timestamp has 200 transactions, all 200 get added before the check runs. Should the check be also inside the inner loop for stricter limiting?
There was a problem hiding this comment.
yeah, I can put it in the inner loop
| // because some managers on mainnet have already hit the limit and we need to batch them | ||
| // to make sure they get cleaned up properly | ||
| // This will be removed eventually | ||
| if transactionsToRemove.length > 50 { |
There was a problem hiding this comment.
This limit is only temporary. The cases that we've become aware of going over the computation limit were in the 100s of iterations, so we believe that this should be sufficient to allow them to clean up without hitting the limit again. We'll be doing some more testing also to make sure
There was a problem hiding this comment.
I believe this number should be the same as the max transactions that can be scheduled per time slot (technically per block).
There was a problem hiding this comment.
I feel like that number might be too high because this is just per user and can include transactions for a lot of previous timestamps. One of the partners whose transactions were failing was failing with only 150 transactions previously scheduled, so I think we need to keep the limit lower than that to make sure we don't hit their computation limit again, especially if they are doing a lot of other complex stuff in their transaction
| // because some managers on mainnet have already hit the limit and we need to batch them | ||
| // to make sure they get cleaned up properly | ||
| // This will be removed eventually | ||
| if transactionsToRemove.length > 50 { |
There was a problem hiding this comment.
I believe this number should be the same as the max transactions that can be scheduled per time slot (technically per block).
Fixes an issue in
FlowTransactionSchedulerUtils.Manager.cleanupwhere we were not removing old timestamps from the array when their transactions were cleaned up. This was causing that array to grow without bounds and causing some apps to reach the computation limit.Additionally, I removed the check for the transaction status to be
nilor Unknown before removing because this is not sufficient to remove old transactions in time. We originally included this because we thought it might be useful for people to be able to query their executed transactions for their status, but the highest priority is actually to keep the computation limit low and apps can figure out different ways to monitor their transactions.There is also the issue that some of these managers have already reached the computation limit, so if we just changed this check, it may not actually work to get those into a working state. Because of this, I included a conditional in the cleanup function that breaks the loop after 100 ids so that the old timestamps can be cleaned up in batches until it is in a manageable state. Then we can remove that check in an upgrade. Not sure it 100 ids is the right number because I just picked an arbitrary one. I'm open to feedback on other options for number of ids or other ways to batch it.
Another option would be to remove the cleanup function from
schedule()entirely, but then there is no way to keep the maps from growing indefinitely without calling cleanup manually, so I would still like to keep it in there if possible.