E2E Are Failing Due To The Debug Log Entries

zutigrm commented 2 months ago

Feature Description

There has been 2 E2E errors that made it into the develop branch recently. Earliest occurrence seems to originate from here https://github.com/google/site-kit-wp/actions/runs/10713306632/job/29771268460

Error:

Entries found in the WordPress debug log: ["[06-Sep-2024 08:32:34 UTC] Cron reschedule event error for hook: googlesitekit_cron_update_remote_features, Error code: could_not_set, Error message: The cron event list could not be saved., Data: {\"schedule\":\"twicedaily\",\"args\":[],\"interval\":43200}"]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

E2E should not fail due to cron errors

Implementation Brief

This issue doesn't seem to be connected with our implementation, as it is not reproducible locally (tried manually to reset/deactivate plugin, tracked events, run few times, then repeated, etc), and this is more likely connected with the test environment. And something similar seems to be a bug occasionally occurring from WordPress side only on some environments, or might be a DB connected, etc.

[ ] It is safe to ignore this error in the log. So in tests/e2e/config/wordpress-debug-log/log-ignore-list.js
- Include Cron reschedule event error for hook: googlesitekit_cron_update_remote_features to the arrays, under nightly and latest (it needs to be added as a new array key, currently not present)

Test Coverage

No updates needed

QA Brief

No QA needed; if the E2E tests are passing this can be moved directly to Approval.

Changelog entry

N/A

aaemnnosttv commented 2 months ago

@zutigrm looks like it's coming from this when attempting to update the cron option. Did we change anything to touch this directly? https://github.com/WordPress/WordPress/blob/00a39b7510b97b76c86768b0c0ba4ecb9797534b/wp-includes/cron.php#L1243-L1250

zutigrm commented 2 months ago

@aaemnnosttv from what I see, we do not change it directly, but seems to me on the first glance on the PR where these failures started, that it might be the matter of timing, when the scheduled events are cleared out. Could be that there is a schedule run after the events have been already cleared

aaemnnosttv commented 1 month ago

Thanks @zutigrm – that looks like a likely culprit 👍

benbowler commented 1 month ago

I've verified that removing uses of wp_clear_scheduled_hook appears to fix this specific test failure. I'm looking into the steps that cause the error to be logged and how we can make sure it doesn't happen. The error catching code in _set_cron_array was added in 5.7.0 which is why it's occurring in nightly and latest not 5.2.

tofumatt commented 2 weeks ago

IB ✅

aaemnnosttv commented 2 weeks ago

@tofumatt While the ignore is probably necessary for now, we should also update the way the crons are cleared to use a different function. I tried this before in https://github.com/google/site-kit-wp/pull/9431/files and it didn't fix this issue as I thought it might, but is still something we should do.

The difference is that the current function is specific to unscheduling cron tasks with a specific set of arguments, and the latter is independent of args (which is what we'd want).

zutigrm commented 2 weeks ago

@aaemnnosttv I will update the function as well as part of this PR, since it is a small change

eugene-manuilov commented 1 week ago

Moved straight to Approval since there is nothing to QA there.

google / site-kit-wp