Display and delete invalid background jobs

[phil-davis]

Description

If I have background jobs that are from an old version of an app (their associated class no longer exists) or are from an app that is now disabled (and maybe deleted from the server) then occ background:queue:status does not show them (the code of that command only reports background jobs where the class of the job can be instantiated). So I can't find out the job id of those invalid jobs.

Even if I have found out the job id, if I try occ background:queue:delete with the job id, it tells me the job id is not known, because the code only finds a job id if it has a class that can be instantiated.

It's a PITA.

This PR adds:
occ background:queue:status --display-invalid-jobs
so that the user can see the invalid jobs.

And the occ background:queue:delete is adjusted so that, if the job id exists then it is deleted (whether or not the job is valid).

How Has This Been Tested?

TBD

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[phil-davis]

@cdamken I noticed that occ background:queue:status does not display invalid jobs, and that occ background:queue:delete does not find and delete them. I have fixed that with this PR.

How do installations usually cleanup left-behind background jobs?

[ownclouders]

💥 Acceptance tests pipeline apiSharees-maria10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38643/53

[jvillafanez]

I think it's better if we use a listJobsIncludingInvalid(\Closure $callback) method instead (I'm not sure about the callback parameter, so maybe we can remove it). The behavior can be copied from the listJobs(...) method, but taking into account the invalid jobs.
The new listJobsIncludingInvalid(...) method will return a list of valid jobs and another list of invalid jobs, any of them could be empty.

This should be a better approach because the "status" background job only needs to do one query to the DB, and it also needs to query the DI once too.

[phil-davis]

I refactored, removing listInvalidJobs, and made listJobs take an optional parameter $onlyInvalidJobs

The "tricky" thing is that for invalid jobs it is not possible to create a $job that is really the class of the invalid job. So I create it as a CommandJob class, and the callback has a 2nd parameter where it can pass the "invalid" class name as a string, which ends up in the command output. (It is not helpful if the command output always just says CommandJob for invalid jobs)

See PR #40848 - is that a better thing to do?

[jvillafanez]

I'd still keep my idea.
The reason to use a different method is to avoid modifying the current listJobs method. As you've said, we can't build a job from an invalid class, so we can't return anything useful (adding a FakeJob might be cumbersome)
By adding a new method, we have the flexibility to return anything we want. For this particular case, we can return a list of IJob for the valid jobs AND a list of DB rows for the invalid jobs.
We could also return all the DB rows, but it might be less useful because we can't build the jobs if needed from outside the JobList

[jvillafanez]

returning the whole row seems better because it gives more information at the same price, so you can either return the row, or null if the row doesn't exists. Maybe it shouldn't be called jobIdExists then.

[phil-davis]

jobIdExists is only used by Delete.php. Delete.php has a Job Id and only needs to know a true/false answer about if the Job Id exists. Then it deletes the job. It does not use any information about the job.

But if we add some interactive confirmation dialog, then it would be useful to have the data about the job.

I will make a proposal on top of this for some sort of "confirmation dialog" and "force" option.

[jvillafanez]

Should we include a safety check somehow? Maybe preventing deleting valid jobs unless explicitly asked. Something like "This job is currently valid. Use the --force option if you want to delete it"

[phil-davis]

The current code deletes a valid job without confirmation.
If a --force option is to be a requirement, then that is a new requirement.

The current code does not find an invalid job (a job that is in the database but the class of the job no longer exists, cannot be found). And so there is no way to delete an invalid job. This change just allows invalid jobs to be found and deleted.

[phil-davis]

@jvillafanez please review again. --display-invalid-jobs now does "Also display jobs that are no longer valid" and there is method listJobsIncludingInvalid to do that in one call.

[jvillafanez]

Callback documentation is important. We might not need to change the code (maybe for the error handling in the callbacks), but we need to ensure the expectations and responsibilities are clear between what core provides and the callbacks.

[jvillafanez]

Need documentation about the callbacks. What parameters are expected? Can we change the parameters from the callback somehow? What should the callback return? How the errors are expected to be handled?

For example, the $validJobCallback could use an IJob as parameter, or a DB row, or multiple parameters such as a DB row plus an output stream (in order for you to write in the provided output stream instead of the stdout, for example); the $validJobCallback could also return true (a boolean) to stop the listing because I'm only interested in the first 10 jobs.

The error handling is also important because we need to know who need to handle any error that could happen inside the callback. If it needs to be handled by the callback, we need documentation because the callback might need to have a "catch all" policy not to leak any exception to the outside.

[phil-davis]

I added documentation about the callbacks. I added it to the original listJobs method also - that did not document the existing code.

My additional code does the same sort of thing as listJobs already did. I reverse-engineered documentation.

[jvillafanez]

kind of weird that the validJobCallback can stop the iteration for the invalidJobCallback.... I guess it's ok because the caller should be aware, and it's up to him to setup the callbacks correctly.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

75.5% Coverage
0.0% Duplication

[jvillafanez]

kind of weird that the validJobCallback can stop the iteration for the invalidJobCallback.... I guess it's ok because the caller should be aware, and it's up to him to setup the callbacks correctly.