SetUserStatusBasedOnUserCapSettingWorker Contains Defunct Code
The SetUserStatusBasedOnUserCapSettingWorker
contains code that is nearly unreachable except for an edge case. This was noticed while working on #361563.
This chunk of code is almost never reachable, because just above it, we return
if the user cap has been reached:
if User.user_cap_reached?
send_user_cap_reached_email
return
end
However, we invoke this worker only one place in the entire code base, in the User
model in an after create hook, and that code invokes the worker only if User.user_cap_reached?
is true
:
def perform_user_cap_check
return unless self.class.user_cap_reached?
return if active?
run_after_commit do
SetUserStatusBasedOnUserCapSettingWorker.perform_async(id)
end
end
That means that if the worker is invoked, then User.user_cap_reached?
will be true
, and the if
statement in the worker will return
, meaning the code afterwards will never be invoked.
There is one edge case where the code may run.
The worker is low urgency. This means its queue scheduling targets are 1 minute or at worst 5 minutes. So in a typical scenario, the worker may run immediately or it may run anywhere from 1 to 5 minutes after the user is created.
So it is possible that the user may be created while the instance user cap is exceeded, and the worker may be enqueued but not yet executed, and then space for more billable users may free up, possibly through users being deleted for instance, and then the worker may finally execute. In this case, User.user_cap_reached?
is true
when the user is created and false
when the worker executes, and so the code in question in the worker would execute.
But what does this mean? It means that the newly created user was set to the blocked_pending_approval
state at creation time, because the user cap was exceeded, and then within the next few minutes at the latest, free space appeared on the instance and so the user was automatically set to active
without admin intervention. Does this seem like a likely scenario? Does it even seem like desired behavior in this scenario? What is wrong with the user remaining in the blocked_pending_approval
state in this case? The fact is, when the user was created, the instance user cap was exceeded. It would not be surprising or unexpected behavior to find the user in the blocked_pending_approval
state in this case.
Proposal
In Scope of this issue:
- Remove the essentially unused code from the
SetUserStatusBasedOnUserCapSettingWorker
. It does not seem like we ever meant for this to handle the edge case described here, and I do not think it seems worth the extra code complexity to handle. The behavior without this code would make sense, as described above.
Out of scope:
- Rename the worker to something else since it's not really setting the state of the users anymore but rather only notifying the admins that the instance user cap has been exceeded. We will need to pay attention to special considerations for changing Sidekiq worker code when renaming the worker and this may have to be done across multiple releases.
- We could consider refactoring in other ways as we do this work. For instance, we could consider eliminating the worker entirely. Though in this regard, most likely we will want to retain it so we leverage sidekiq and avoid doing too much work in the
after_create
hook inside theUser
model.
We can take care of these two out of scope bullet points in Rename SetUserStatusBasedOnUserCapSettingWorker (#440233).