-
Notifications
You must be signed in to change notification settings - Fork 851
IDC: preserve re-validation parameters #46613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces logic to preserve the re-validation timing parameters (last_checked and next_check_delay) when a new IDC (Identity Crisis) error is generated from a remote request. The changes ensure that if the underlying cause of the IDC remains the same (matching wpcom URLs), the backoff delay is preserved while the timestamp is updated. If the wpcom URLs differ, both timing parameters are reset to initial values.
Changes:
- Updated
check_response_for_idc()to preserve timing parameters when wpcom URLs match - Added
has_same_wpcom_urls()helper method to compare wpcom URLs between existing and new IDC errors - Added comprehensive test coverage for the new timing preservation logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| projects/packages/connection/src/identity-crisis/class-identity-crisis.php | Implements logic to preserve next_check_delay when same wpcom URLs are detected, and resets timing when URLs differ |
| projects/packages/connection/tests/php/identity-crisis/Identity_Crisis_Test.php | Adds four test methods covering preservation, reset, and initial setting of timing parameters |
| projects/packages/connection/changelog/update-idc-preserve-revalidation-params | Adds changelog entry for the feature |
projects/packages/connection/changelog/update-idc-preserve-revalidation-params
Outdated
Show resolved
Hide resolved
projects/packages/connection/src/identity-crisis/class-identity-crisis.php
Outdated
Show resolved
Hide resolved
projects/packages/connection/tests/php/identity-crisis/Identity_Crisis_Test.php
Outdated
Show resolved
Hide resolved
fgiannar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving after our Slack convo: p1768559411003029-slack-C05PV073SG3 👍
@sergeymitr If you could also take a look at this one before we merge I'd appreciate it.
sergeymitr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me, just a small improvement we might want to do.
If the site is messed up enough to trigger this code, it won't hurt to do additional checks.
| // Same wpcom URLs - preserve the backoff delay. | ||
| // Note: last_checked is already set to time() by get_sync_error_idc_option(), | ||
| // which is correct - we want to record that we just saw this error again. | ||
| if ( isset( $existing_idc['next_check_delay'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to check that the value is a valid in case it gets corrupted: a number, and between IDC_VALIDATION_INITIAL_DELAY and IDC_VALIDATION_MAX_DELAY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. I added the validation and refactored the code a bit for clarity.
3436d94 to
1bd4131
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
projects/packages/connection/changelog/update-idc-preserve-revalidation-params
Outdated
Show resolved
Hide resolved
projects/packages/connection/src/identity-crisis/class-identity-crisis.php
Outdated
Show resolved
Hide resolved
projects/packages/connection/src/identity-crisis/class-identity-crisis.php
Outdated
Show resolved
Hide resolved
sergeymitr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested using some in-code hacks, works well 👍
The new re-validation logic for IDC introduced two new parameters in the IDC error array that define the last checked timestamp and the progressive delay for the next validation attempt. This PR adds logic that makes sure these two parameters are inherited when a new IDC error is generated (by a new remote request).
Closes CONNECT-139
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: