Old-item failsafe logic for digests

[boonebgorges]

When running properly, digests should only send out items that were created in the prior period appropriate for that particular digest. That is: daily digests should only contain activity items from the past 24 hours, and weekly digests from the past 7 days.

There are a few kinds of situations where this might not hold true. Digest runs may fail for one reason or other: wp-cron might run late or fail altogether; the process may time out because of large numbers of users/items, such that some users get their digests but later ones do not; miscellaneous server failures; etc. When this happens, the digest queue for any user whose digest did not send will carry over to the next digest. In this sense, the current behavior might be considered a feature and not a bug, since it ensures that a user whose digest isn't sent one day ought to get those items the next time a digest goes out.

There's also the possibility of queue corruption. If activity-related hooks are somehow fired a second time, they'll be readded to queues. If there are database replication issues, digest items might be fired more than once. And so forth. In this case, it's a Bad Thing when the old items are sent.

The worst-case scenario - which was recently reported to me by a user - is that users end up getting digests that contain very old items that they've already received. In this case, all of the user's members got a digest containing every item to which they've ever subscribed in the history of the site :-D

Some sort of time-based failsafe seems wise to me. Something like: When generating a daily digest, check the timestamp on each activity item; if it's older than (say) three days, delete it from the queue and don't include it. (Likewise for weeklies.) The failsafe time period could be filterable.

Objections or ideas from the peanut gallery? cc @rekmla @modelm who are the unfortunates who first reported the problem to me

[modelm]

It would make sense to have these options alongside "Member must be registered for N days".

I think 3x the digest frequency is a good default max age i.e. 3 days/daily, 3 weeks/weekly.

It might also be useful to have a total max number of items per digest, and/or per group.

[boonebgorges]

I'm wary of adding more UI, mainly because it's a bunch of work to build and confusing to explain. The problem we're trying to solve should, in theory, never arise, and when it does, it'll only be exposed by someone who was digging into the codebase. Maybe we can start with the filter, and add a UI setting if people ask for it.

This does remind me, though, that the $type should be passed along to the filter. The stale period will obviously be different for weekly than for daily digests.

It might also be useful to have a total max number of items per digest, and/or per group.

Can you describe the situation where this would save someone a headache, where the "stale" feature would not? I guess if there were three days in a row with hundreds of posts, where the sending got hung up for two days? My concern here is that there are many legitimate situations where a digest would have a large number of items, and default ceilings are just as likely to introduce bugs in those scenarios.

How's about this for a broader solution. Instead of a simple, inline check against the "stale" period, we break this out into a standalone function along the lines of: bp_ges_check_digest_item(). By default, the only thing we'd check there is staleness. But there'd be a filter on the return value, so that anyone who wanted to cap based on overall number (or anything else) would be able to do so. Does that sound sensible?

[modelm]

Can you describe the situation where this would save someone a headache, where the "stale" feature would not?

I was thinking of preventing very large emails from going out regardless of staleness of individual items. Many (most?) email clients have a max size limit and will cut off portions of emails that exceed that limit. Limiting the number of items that can appear in a given digest is one way to mitigate that. I think this use case stands apart from preventing stale digest items so maybe it belongs in a separate ticket, but it seemed related.

Instead of a simple, inline check against the "stale" period, we break this out into a standalone function along the lines of: bp_ges_check_digest_item()

Totally fine by me, this covers everything I can think of without overcomplicating it.

[boonebgorges]

@modelm I've just pushed up some updates to the pull request.

On further thought, I wonder if this new "valid" filter will fulfill what you're asking for in #125. What do you think?

[modelm]

I don't think this filter would work for that purpose unless it can stop the entire digest from being sent - seems like this would only be able to remove individual activities (which I guess could be abused to remove everything but that seems like the wrong way).

[boonebgorges]

@modelm Yeah, after I posted that question, the same thing dawned on me - it'd be an abuse of the fliter. Let's merge this one, and handle #125 separately.

[boonebgorges]

Fixed in #126.