WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#9388 closed defect (bug) (invalid)

safety guard for publish_future_post

Reported by: hailin Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.7.1
Component: General Keywords:
Focuses: Cc:

Description

There were reports that scheduled future post won't publish.
And once it missed the scheduled time, the Edit=>post admin page
display orange "Missed Schedule". However, there is no other trace left to determine what happened. There is no cron entry left in the options table.

I've been tracing this bug for a few weeks.

And have determined that for a missed schedule, spawn_cron() is never called in the first place. That means the cron entry wasn't in the table.

For two possible reasons:

  1. cron entry wasn't successfully added when user first schedule a post. Eg, network timed out, db issue. However, post_date, post_date_gmt were modified in db. So user would perceive the post has been successfully schduled, although cron entry wasn't there.
  1. cron entry was erased in one of the many _transition_post_status

And it wasn't added back while the post_status is 'future'

I've spent many hours trying to reproduce it without luck.

It usually happens on busy sites with thousands of daily page views.
and it happens rather randomly. Such as once per week.

So there is a need to make sure when we display "Scheduled for..",
there is indeed a cron entry there. Add it if not.
This at least fixes case 1.

Attachments (2)

9388_cron.diff (1.0 KB) - added by hailin 5 years ago.
patch
9388_schedule.diff (658 bytes) - added by hailin 5 years ago.
patch for another place

Download all attachments as: .zip

Change History (18)

hailin5 years ago

patch

hailin5 years ago

patch for another place

comment:1 follow-up: hailin5 years ago

Some users reported that it appears to only happen if he edit the sheduled post after sheduling it. That points to the potential issue in _transition_post_status in which we ALWAYS deletes the existing cron entry.

9388_schedule.diff avoid deleting publish_future_post hook when transitioning post.
This certainly will leave some stale entries in cron options table, but that is fine. In check_and_publish_future_post($post_id), we do nothing when the status is not 'future'

if ( 'future' != $post->post_status )

return;

So I believe it is better to leave a few stale entries in the table, then mistakenly delete a valid one (and never add it back due to other circumstances). Users are pretty stressful and (angry) when their scheduled posts failed to show up and they have to manually intervene.

comment:2 in reply to: ↑ 1 ryan5 years ago

Replying to hailin:

Some users reported that it appears to only happen if he edit the sheduled post after sheduling it. That points to the potential issue in _transition_post_status in which we ALWAYS deletes the existing cron entry.

_transition_post_status fires and then _future_post_hook fires when resaving a scheduled post, both of which clear the scheduled hook.

The issue of cron contention is well known. I think this is just a case of that. We were planning to split the cron array into separate options for 2.8, but it hasn't happened yet.

comment:3 hailin5 years ago

Yeah, I saw the cron contention. It's easy to reproduce the symptom that one publish_future_post is fired and handled twice on two different Web servers concurrently.

However, the issue in this ticket is about lost publish_future_post cron entry in options table. It's very likely it was lost in one of the more subtle status transitions (autosave, etc). Basically I propose not to delete cron entry during transition to avoid accident.

comment:4 lloydbudd5 years ago

  • Milestone changed from Unassigned to 2.8
  • Version set to 2.7.1

comment:5 lloydbudd5 years ago

  • Cc has-patch added

comment:7 hailin5 years ago

Transport was another potential cause for failed cron.
But this ticket is not aiming for that.

It's about the missing cron schedule in the DB for a future post. Usually after user heavily edited the post and went through several cycles of save, autosave, etc, the publish_future_post cron entry was lost in the DB.

I will run this patch on a live site for a few days or two weeks and see how it goes.

comment:8 westi5 years ago

If we are losing cron entries then it is likely due to the contention issues.

Patching around the problem isn't really the best idea.

Better to split the cron stuff out.

We need a good design for that.

One interim solution could be to use post_meta for post_related cron options.

comment:9 hailin5 years ago

Before we switch to a new solution, according to my experiments, this patch helps to recover from or avoid some missing cases.

I agree that the root cause may be in the stale cron array cache, and rethinking of the paradigm is needed.

comment:10 follow-up: hakre5 years ago

westi: what should a cron subcomponent do? should be no deal to code one.

comment:11 in reply to: ↑ 10 ; follow-up: westi5 years ago

Replying to hakre:

westi: what should a cron subcomponent do? should be no deal to code one.

What we need is a good generic design for storing the cron entries.

Each one needs to be stored independently in the database so as to avoid all the caching collision issues that we see on adding/removing of cron entries which cause some to get lost on busy sites / multiserver environments like wp.com

We need to work out where the best place to store these is.

Is it a new table or should we be storing things in an existing/new meta place??

A proposal for the storage of the cron entries would be a good place to start. Migrating the code should be simple but we need to get the storage design correct first.

comment:12 in reply to: ↑ 11 hakre5 years ago

Replying to westi:

Replying to hakre:

westi: what should a cron subcomponent do? should be no deal to code one.

What we need is a good generic design for storing the cron entries.

Each one needs to be stored independently in the database so as to avoid all the caching collision issues that we see on adding/removing of cron entries which cause some to get lost on busy sites / multiserver environments like wp.com

We need to work out where the best place to store these is.

Is it a new table or should we be storing things in an existing/new meta place??

A proposal for the storage of the cron entries would be a good place to start. Migrating the code should be simple but we need to get the storage design correct first.

ideally a new database table can do the job. re-using any of the existing ones does not make so much sense to me, wp_options doesn't seem fitting as well as it isn't wp_postmeta. mainly because of the index design, should get fast access for inset, update and delete. if it should be used for WP.com i think a blogid must be provided as well like in the options?

comment:13 hailin5 years ago

Please ignore the original patches, since it appears this does not fix the root cause of the cron issue.

It helps to recover from one edge case, but I too hate to see any single line of redundant code.

comment:14 hakre5 years ago

  • Milestone changed from 2.8 to Future Release

comment:16 ryan5 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Since original reporter says to ignore patches, closes this as invalid. We have plenty of other tickets about cron and know we need to work on the options storage.

Note: See TracTickets for help on using tickets.