Right approach to handle pending queue jobs at upgrade

[touhidurabir]

With the release of 3.4.0 , we have introduced the queue jobs to handle long running process in background . These jobs are stored in database(set driver, currently only support database driver) in serialized format and later retrieved from database and reconstruct the jobs class from the serialized data . As for these sole reason, it is important to maintain the consistency of the job class structure through out the application release cycle . With the slight change of the jobs class structure e.g. the changes/remove/add of class properties may cause the jobs not the execute it's desired action .

However with each application version release cycle, with the continuous update/bug fix/feature addition to the application, it's normal for job class to end up with different class structure based on requirement. Now if a pending job with different class structure remains in the driver and then in the upgraded version it try to execute that version where the jobs class structure has changed , it may cause the jobs not to be able to execute and fail continuously .

To overcome the above issue we need to have some mechanism so that installation does not end up with older/previous (before update) pending jobs in the system that continue to fail before up change in class structure and cause undesired outcome .

Solution 1:

Before upgrade, we can force to complete the execution of any pending jobs in the system . we have that mechanism built in and a check the preflight check will ask to execute any pending jobs in the system and will not allow to upgrade if there are any pending job in the system .

Solution 2:

We will never touch any of the exiting jobs class but if needed to update the job class structure, have different job class such as JobClass, JobClass_2 etc . This way a previous version such as JobClass will continue to execute . if after upgrade, the previous version is not needed to run or if may cause issue when it run, we can simply update the handle method and just return from there . However that implies that developer take proper care to make sure they do not make any changes to current job class structure, only update the handle method as need to facilitate the update and create a new one to handle updated requirements .

Solution 3:

At the upgrade time, we will check each of the pending job class with the existing class structure and if any mismatch found , try to update the serialized data to make it compatible with the new class structure . This process can be done in upgrade preflight check . However this will be a CPU/Memory intensive task and hard to determine the possible compatible values for update/added properties in job class .

[jonasraoni]

• #3: This would be the best option for a sysadmin, but I think it's too risky to give maintenance for us. N updates might be done to a class, including during minor upgrades, and all of them should be covered by the migrations, without mistakes.
• #2: This will also require some attention, and it will also leave some trash behind unless we drop upgrade code in the future.
• #1 I think this is the safest option, and I'd take it. In general users should shutdown a system gracefully before upgrading it.

Extra notes

For the #1 and #3, even if we're careful about our structures, internal changes to Laravel might leave us broken (our job class isn't a plain old PHP object).

Palliative for #1 and #3

If we only use native structures (integers, strings, array, ...), serialization issues should be highly reduced on our side.

[jonasraoni]

Besides the jobs, @asmecher has mentioned that we have other types of serializations (perhaps at the invitations?!), I don't have a clear opinion about options without seeing the code.

The cache is also a good source of such problems, but I think it was already addressed.

[asmecher]

Yes, invitations will also require us to solve the same problem, but we won't be able to ask the admin to resolve all invitations ("have all reviewers accept or reject their invitations") before an upgrade. So even if we adopt solution 1 here, we'll still have to solve the same problem another way for invitations. I propose we use the same solution for invitations and jobs, so that would exclude solution 1 here.

[asmecher]

I started to tinker with a complicated runtime solution involving traits that define exactly what gets serialized for a job. This would signal to the coder that they need to take care when changing the trait that defines the serialized data.

However, it got complicated and started to introduce some "magic" behaviour of a kind I don't like.

If all we need to do is catch a coder who inadvertently changes serialization expectations of a job or invitation, breaking older data, why don't we just solve it through unit tests?

Idea: When we introduce a new job or invitation, we make sure to add a test that includes its serialized form. If the implementation later changes in a breaking way, an exception or error should be thrown, catching the issue before release.

Example: A unit test for the UpdateSubmissionSearchJob.

<?php

/**
 * @file tests/jobs/submissions/UpdateSubmissionSearchJobTest.php
 *
 * Copyright (c) 2024 Simon Fraser University
 * Copyright (c) 2024 John Willinsky
 * Distributed under the GNU GPL v3. For full terms see the file docs/COPYING.
 *
 * @brief Tests for the submission search reindexing job.
 */

namespace PKP\tests\classes\core;

use PKP\tests\PKPTestCase;
use PKP\jobs\submissions\UpdateSubmissionSearchJob;
use Mockery;

class UpdateSubmissionSearchJobTest extends PKPTestCase
{
    /**
     * Ensure that a serialized job can be unserialized and executed
     */
    public function testRunSerializedJob()
    {
        // Serializion from OJS 3.4.0 for a submission reindex job with submission ID 17
        $updateSubmissionSearchJob = unserialize('O:46:"PKP\jobs\submissions\UpdateSubmissionSearchJob":3:{s:15:"^@*^@submissionId";i:17;s:10:"connection";s:8:"database";s:5:"queue";s:5:"queue";}');

        // Ensure that the unserialized object is the right class
        $this->assertInstanceOf(UpdateSubmissionSearchJob::class, $updateSubmissionSearchJob);

        // Mock the Submission facade to return a fake submission when Repo::submission()->get($id) is called
        $mock = Mockery::mock(app(\APP\submission\Repository::class))
            ->makePartial()
            ->shouldReceive('get')
            ->with(17) // Submission ID from serialization string
            ->andReturn(new \APP\submission\Submission())
            ->getMock();
        app()->instance(\APP\submission\Repository::class, $mock);

        // Test that the job can be handled without causing an exception.
        $updateSubmissionSearchJob->handle();
    }
}

Notes:

• Requires the developer to implement a test when the job is created, but this is a good idea anyway.
• Will catch breaking changes caused by:
  • Job unserialization no longer working (this is the main case we want to prevent)
  • Expectations in the job handling class changing in a breaking way (they should be checked in the handle() function with exceptions)
• Allows the job class to evolve without adding complexity
• Testing happens where it's supposed to, rather than at runtime.

[asmecher]

@touhidurabir, will the unit testing not catch this scenario when the $f property is added? In that scenario, when the developer adds the job, there will be a test with a serialized job coded into it with $a and $s but not $f; when the new requirement for $f gets added, it will break the test because the handle() function will attempt to use $f before it is defined.

[jonasraoni]

Hmm, I think the tests are enough to cover serialization issues, but optional attributes can be risky though (including the ones from parent classes), I mean a class x {public string $y;} that later becomes class x {public int $y;}. If the tests don't use the optional stuff, the problem won't be caught.

So, as you prefer to have a unified approach, I think what remains is the option #2... Basically to avoid at all costs changing the class name/namespace/attributes, but if needed, namespace/class name versioning must be introduced.

Keeping old code should also protect the plugins, which will still be able to create deprecated jobs.

[touhidurabir]

will the unit testing not catch this scenario when the $f property is added

@asmecher it will do given that developer will not update the tests to make it compatible with new updated job class . So basically it will be developer's responsibility not to update the test and if test fails and need to make proper adjustment or create new version to job itself. A bit big responsibility imposed on developer but probably the right way to do it .

@asmecher @jonasraoni if I am understanding correctly , we plans to go for Solution 2 which I think much better and probably the right one for us .

[asmecher]

@touhidurabir and I talked this through today and agreed on some things -- let me know if I missed/mischaracterized something, Touhidur!

• Generally better unit tests are a good thing, and including a serialized form as proposed above would help catch later evolutions of the jobs when the developer forgets to handle upgrades. I've adopted that pattern (see new -j and -J options to lib/pkp/tools/runAllTests.sh and tests/jobs directories).
• We generally agree that Solution 2 will be more future-proof than solution 1 or solution 3. But it is up to individual jobs (and invitations) to make informed choices about the patterns they follow -- it's possible that someday we'll have a use case that's not well served by Solution 2.

[jonasraoni]

Okay, so I think this discussion can be moved to Accepted, unless other members want to give their opinion.