Re-enable any queues that were disabled for background processing, on end

[eileenmcnaughton]

…

Overview

This addresses what is likely an artificial scenario but

essentially if these steps are followed twice in separate processes

• create a queue
• run the queue via runAll

The second time will fail because the queue was disabled & no re-enabled, despite being perisistent

Before

After

Persistent queues are more re-usable

Technical Details

Comments

[civibot]

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[totten]

Yeah, I think it's fair/symmetric to say: "We're only disabling background while the foreground thing is active." Agree it's a kind of a funny/unlikely scenario. If you're happy with it and it passes, then go ahead and merge.

Just to verbalize some characteristics of this approach:

• If the runAll() crashes, then you're stuck in disabled state until an admin intervenes.
• The runAll() and runAllViaWeb() would behave differently. This only restores background for runAll().
• If you want both of them to restore background, then maybe put the new call from runAll() to handleEnd(). (That method should be used by both.)

[eileenmcnaughton]

@totten yeah - I wasn't sure about putting it in handleEnd but I am open to moving to handleEnd - note I managed to replicate this in a crm only test - it required some flushing that is a bit artificial but I guess the use case is having a persistent queue that may or may not have items in it at any point in time

[eileenmcnaughton]

Actioning " If you're happy with it and it passes, then go ahead and merge."

[totten]

I guess the use case is having a persistent queue that may or may not have items in it at any point in time

Just to verbalize more, because this comment didn't connect for me.

In my mind, I comprehend 4 main scenarios -- based on mixing two main variables:

• Persistent vs ephemeral (new style vs old style)
  • Is $queue stored in civicrm_queue?
  • There's no real advantage to the ephemeral style AFAIK, except for continuity for older callers.
• Background service vs transactional batch (outbound_mail vs import_1234).
  • Do we handle an open-ended task-list forever? Or do we handle a specific task-list for short timeframe?
  • These can have different implications for how you comprehend configuration, logging, etc.

Mixing those, we get the 4 cases:

Case	Definition	Typical Execution Method	Additional Notes
Persistent background service	Persistence: civicrm_queue Name: outbound_mail	APIv4 Queue (called via cron or coworker)
Persistent batch run	Persistence: civicrm_queue Name: import_1234	CRM_Queue_Runner->runAllInteractive()	Internally may involve other methods (e.g., runAllViaWeb(), APIv4, coworker, etc).
Legacy batch run	Persistence: n/a Name: import_1234	CRM_Queue_Runner->runAllViaWeb()
Legacy background service	Persistence: n/a Name: outbound_mail	Varied	This was only done in contrib. I think there were a couple variations on it.

I interpreted your goal/scenario as "Write phpunit tests involving a persistent background service". If I were trying to do that, how would I have approached testing?

1. runAll() probably wouldn't have entered my mind. A persistent background service has no end (there is no thing such as "all" unless you're going to do an infinite sleep-poll loop, which you don't want in a test).
2. Background services run via APIv4 Queue.runItems (via cron/coworker).
3. So I probably would've landed on something like:

   function testMyQueue() {
     Civi::queue('foo', [ .... 'batch_limit' => 9999...]);
     ...
     Civi\Api4\Queue::runItems()->setQueue('foo')->execute();
   }

Relatedly -- Queue.runItems is the envisioned as the normal thing for a background service, so it doesn't muck around with disableBackgroundExecution().

(None of that's a criticism. I can see how one might interpret runAll() to mean "run every item that's currently available" -- so then you would think to use CRM_Queue_Runner->runAll(), esp if you'd used it before for batch-runs.)

[eileenmcnaughton]

@totten thanks - I picked up runAll from the docs https://docs.civicrm.org/dev/en/latest/framework/queues/#1-creating-a-queue & an existing unit test usage