Past-due tab seems not to be working correctly with asynchronous actions

[piotrbak]

We use Action Scheduler to register many async actions at the same time. Since 3.5 update of Action Scheduler all our actions are set to past-due just after being scheduled and it's very confusing for our customers.

It looks like this update doesn't play well with async actions as the date for them can't be set:

array (
  'hook' => 'async_request_url',
  'status' => 'pending',
  'scheduled_date_gmt' => '0000-00-00 00:00:00',
  'scheduled_date_local' => '0000-00-00 00:00:00',
  'schedule' => 'O:28:"ActionScheduler_NullSchedule":0:{}',
  'group_id' => 10,
)

Is that something you are aware of, or maybe we're missing the point here?

[barryhughes]

Hi @piotrbak,

We did consider this point, but decided to ship and see what feedback might come in—thank you for sharing—since in some cases this may be less of a problem, or not a problem at all (and, of course, an async action can itself correctly be considered as past-due until it has been processed).

Given your note, and the reactions it has already accrued, though, we can look into adjustments.

[piotrbak]

Thank you @barryhughes for the quick response, much appreciated.

We decided not to update to the latest AS version before it passes our QA process, but it was updated on the other plugins right away 😁 In the last three days our support team was flooded by confused customers asking questions about what's happening. For now, we decided to hide the Past-due tab with a filter for our customers until we can find a less confusing approach. Maybe the async action could be treated as past-due after a specific time since it was created?

We'll be more than happy to contribute and make a PR that would improve this specific enhancement, if that makes things faster. Let me know what do you think.

[barryhughes]

Maybe the async action could be treated as past-due after a specific time since it was created?

That's a possibility, and is probably the best course of action (though I suspect will also need an accompanying schema change). A simpler change—perhaps as an interim measure—could be to remove async actions from the Past Due view, and/or move them into their own view.

For now, we decided to hide the Past-due tab with a filter for our customers until we can find a less confusing approach.

Thanks for sharing that— it could be useful to others until we address this.

We'll be more than happy to contribute and make a PR that would improve this specific enhancement, if that makes things faster.

That would be welcome, and if you want to outline your proposed solution here first, before creating the PR, that is also fine.

[engahmeds3ed]

@barryhughes what do u think about the following grooming:

Here: https://github.com/woocommerce/action-scheduler/blob/f2abbce98bcde7113a8c9bcb3e3dae2ee6a2cae3/classes/data-stores/ActionScheduler_DBStore.php#L435 when the date is sent, we may replace this line with the following:

$sql         .= " AND a.scheduled_date_gmt != 0 AND a.scheduled_date_gmt $comparator %s";

this will make sure that we will only compare when the field has a value on it.

If u agree on that, I will create a PR for it.

More context

Here https://github.com/woocommerce/action-scheduler/blob/6640736f7834cf7b5ba8986ee400c399ba53cf82/classes/ActionScheduler_AdminView.php#L154-L158

we send the query parameters to get past-due actions here https://github.com/woocommerce/action-scheduler/blob/6640736f7834cf7b5ba8986ee400c399ba53cf82/classes/ActionScheduler_AdminView.php#L163

Which will call this one https://github.com/woocommerce/action-scheduler/blob/f2abbce98bcde7113a8c9bcb3e3dae2ee6a2cae3/classes/data-stores/ActionScheduler_DBStore.php#L525-L532

and then call the method get_query_actions_sql to prepare the SQL query

[barryhughes]

This will make sure that we will only compare when the field has a value on it.

Well, this would be a breaking change. Consider the following snippet—a plugin implementing this will get different results before and after implementing the proposed change:

# Currently, this will also fetch async actions.
# With the proposed change, it will exclude async actions.
$actions = as_get_scheduled_actions( [ 
    'date'         => wp_date( 'Y-m-d H:i:s' ), 
    'date_compare' => '<',
    'per_page'     => -1,  
] );

Also, it would make fetching only async actions (via date and date_compare args) impossible—though we could of course tweak the logic to accommodate this.

This makes me think it might be preferable to add support for a new query arg such as include_async_actions, which would be (bool) true by default for backwards compatibility reasons. That way, we open up the ability to exclude async actions and we do not introduce any breaking changes.

What do you think? (cc @jorgeatorres as you worked on the related change.)

[engahmeds3ed]

I believe u r totally right, I didn't consider this in my approach. But the snippet u mentioned is used to return all tasks that are scheduled before now but it will return also all Async tasks without taking the date into consideration because Async tasks don't have a schedule date.

Also, I believe as_get_scheduled_actions should deal only with actions that have a schedule not Async ones.

So I believe we have two options here:

1. Providing this new query argument (BTW, it's a brilliant idea) to be in safe side but we will still have the same problem of getting all Async tasks from the snippet above.
2. Provide another function for getting Async actions.

to solve the main problem of getting all Async, users can provide the status query argument (for the provided snippet example).

what do u think?

[barryhughes]

As a general position, the nature of Action Scheduler leads me to strongly err on the side of backwards compatibility—since it is not uncommon for plugin A to ship with an earlier version of the library but find itself executing against a more recent version as shipped by plugin B.

Providing this new query argument (BTW, it's a brilliant idea) to be in safe side but we will still have the same problem of getting all Async tasks from the snippet above.

I may be misunderstanding here, but it would still be possible to get all async tasks (and, optionally, only async tasks) if we support this new option of include_async_actions since the date component of the query could be set to match actions where the scheduled date is exactly 0000-00-00 00:00:00.

Conversely, it would become trivial to exclude them. So, all three pathways (fetch only async actions, fetch all types of action, fetch only non-async actions) would be viable.

To solve the main problem of getting all Async, users can provide the status query argument (for the provided snippet example).

Apologies again, but I'm just a little unsure what you mean. Until it has been processed, a new async action has a status of pending, just like any other new action. Do you mean some kind of new 'pseudo-status' such as async?

[barryhughes]

...At the risk of going a little off-track, it also occurs we could support more complicated date queries:

as_get_scheduled_actions( [ 
    'date_query' => [
        [
            'date'    => '2020-01-01 00:00:00',
            'compare' => '>=',
        ], [
            'date'    => '2021-12-31 23:59:59',
            'compare' => '<=',
        ],
    ],
] );

Which might be generally useful outside of this scenario, too.

[engahmeds3ed]

I may be misunderstanding here, but it would still be possible to get all async tasks (and, optionally, only async tasks) if we support this new option of include_async_actions since the date component of the query could be set to match actions where the scheduled date is exactly 0000-00-00 00:00:00.

Yes, using the date exactly like 0000-00-00 00:00:00 will help getting only Async tasks so yes here we can handle the three possible cases.

Apologies again, but I'm just a little unsure what you mean. Until it has been processed, a new async action has a status of pending, just like any other new action. Do you mean some kind of new 'pseudo-status' such as async?

What I meant is, for the first snippet us used, you wanted to get all tasks that were scheduled before now and as mentioned this will return all Async tasks in between so if the user wanted to get only the old actions, so he can add the status query argument to be like that

$actions = as_get_scheduled_actions( [ 
    'date'         => wp_date( 'Y-m-d H:i:s' ), 
    'date_compare' => '<',
    'per_page'     => -1,
    'status' => 'completed',
] );

so this will return the completed scheduled actions before now + completed scheduled Async actions and will not get the pending Async tasks anymore.

Anyway, I believe we are on the same page here of adding a new query argument to help in filtering the Async actions.

My last question: Is this valid to have the Async task with scheduled datetime value of the time it's created? if we can do that, so no need for anything else? or this conflicts with the separation of concerns concept for this column?

[barryhughes]

...so this will return the completed scheduled actions before now + completed scheduled Async actions and will not get the pending Async tasks anymore.

Gotcha, makes sense :+1:

Or this conflicts with the separation of concerns concept for this column?

The current reality is that the scheduled datetime will always be 0000-00-00 00:00:00 for async actions and it would be extremely unusual for a non-async action to have that value. I don't think that's ideal (for one thing, it leads to async actions being prioritized over past-due non-async actions—which may not always be desirable) so this could change in the future.

In the short-to-medium term, though, I think we can safely rely on this as a tool for identifying async actions (I hope that answers your question?).

[engahmeds3ed]

Awesome, if u agree on making this new query argument, I can do the PR to add that, and make past-due rely on it, OK?

[kaushikasomaiya]

+1

5572862-zen, 5567902-zen

Async and SAs that are delayed by few seconds (and then executed) show up in the past-due notification.

Notice disappears after some time on its own.

[jorgeatorres]

Awesome, if u agree on making this new query argument, I can do the PR to add that, and make past-due rely on it, OK?

Hi @engahmeds3ed! Yes, please (and thank you!). That seems like the best way forward for this.

[engahmeds3ed]

PR is created, thanks.

[barryhughes]

Closing; since 3.5.4 async actions are handled slightly differently.

Though they will often still be included in the 'past due' list, they will not appear ahead of other, older past due actions. This should give a much clearer view of things, but we can of course re-open/create a fresh issue if the situation is still problematic.