-
Notifications
You must be signed in to change notification settings - Fork 876
Plugin Conflicts Guardian: batch the probe across all selected plugins #48402
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2f73a84
Plugin Conflicts Guardian: batch the probe across all selected plugins
arthur791004 957b3dd
Rename pcg_guard_attribute_block → pcg_guard_get_blocked_plugin
arthur791004 df8ac79
Add tests for pcg_guard_get_blocked_plugin and load tester guard rail
arthur791004 0300441
Satisfy Phan: assertArrayHasKey('reason') before assertNotEmpty
arthur791004 db8703b
PCG: trim verbose comments + fix \$status alignment
arthur791004 39711d8
PCG load tester: filter unreadable plugin paths up front
arthur791004 59f6cc9
PCG activation guard: surface batch-level message when attribution fails
arthur791004 16232b1
PCG probe endpoint: skip unreadable entries instead of bailing the batch
arthur791004 63f0394
PCG: update flat-file prefix test to expect undetermined ''
arthur791004 1ecfcc5
PCG activation guard: number-agnostic blocked notice
arthur791004 7147b8f
PCG activation guard: locale-aware list separator in batch-level notice
arthur791004 d4c7cc9
PCG load tester: log probe transport errors to logstash
arthur791004 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
4 changes: 4 additions & 0 deletions
4
projects/packages/jetpack-mu-wpcom/changelog/add-pcg-batch-probes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Plugin Conflicts Guardian: probe all selected plugins together in one loopback request pair so bulk activation cost no longer scales with the number of plugins. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,45 +14,43 @@ class PCG_Load_Tester { | |||||
| const PROBE_TIMEOUT = 15; | ||||||
| const TOKEN_LIFETIME = 30; | ||||||
|
|
||||||
| /** | ||||||
| * Probe mode: file is currently inactive and the endpoint must | ||||||
| * `require` it to exercise its load path. Used by the activation guard. | ||||||
| */ | ||||||
| /** Activation guard: plugins are inactive; endpoint require_once's each. */ | ||||||
| const MODE_ACTIVATION = 'activation'; | ||||||
|
|
||||||
| /** | ||||||
| * Probe mode: file is already loaded by WP's normal bootstrap (it was | ||||||
| * an active plugin before the just-completed update). The endpoint | ||||||
| * must NOT re-`require` the file — doing so would fatal with "Cannot | ||||||
| * redeclare class/function" — and instead just verifies that the | ||||||
| * bootstrap completed cleanly with the new code. | ||||||
| * Post-update healthcheck: plugins are already loaded by WP's bootstrap; | ||||||
| * endpoint skips require_once (would fatal with "Cannot redeclare"). | ||||||
| */ | ||||||
| const MODE_UPDATE = 'update'; | ||||||
|
|
||||||
| /** | ||||||
| * Run the probe against a plugin main file. | ||||||
| * Probe a batch of plugin main files in one loopback request pair. | ||||||
| * | ||||||
| * Fires two loopback requests in parallel: one against `home_url('/')` | ||||||
| * (front-end) and one against `admin_url('index.php')` (so `admin_init` | ||||||
| * fires). The admin probe forwards the current admin's WP auth cookies | ||||||
| * so the loopback can clear `auth_redirect()`. A captured fatal from | ||||||
| * either probe wins; otherwise the front-end verdict is returned. | ||||||
| * Fires front-end + admin probes in parallel; front-end auth cookies are | ||||||
| * forwarded so admin_init can fire. Fatal from either wins; otherwise | ||||||
| * front-end's verdict. On fatal/throwable, the verdict's `plugin` key | ||||||
| * names the file the endpoint was loading at the time. | ||||||
| * | ||||||
| * @param string $plugin_main Absolute path to the plugin's main PHP file. | ||||||
| * @param string $mode Probe mode: self::MODE_ACTIVATION (default) or | ||||||
| * self::MODE_UPDATE. See class constants. | ||||||
| * @return array{status:string,reason?:string,errno?:int,class?:string,message?:string,file?:string,line?:int} | ||||||
| * @param string[] $plugin_mains Absolute paths to plugin main PHP files. | ||||||
| * @param string $mode self::MODE_ACTIVATION or self::MODE_UPDATE. | ||||||
| * @return array{status:string,reason?:string,errno?:int,class?:string,message?:string,file?:string,line?:int,plugin?:string} | ||||||
| */ | ||||||
| public function test( $plugin_main, $mode = self::MODE_ACTIVATION ) { | ||||||
| if ( '' === (string) $plugin_main || ! is_file( $plugin_main ) ) { | ||||||
| public function test( array $plugin_mains, $mode = self::MODE_ACTIVATION ) { | ||||||
| $plugin_mains = array_values( | ||||||
| array_filter( | ||||||
| array_map( static fn( $p ) => (string) $p, $plugin_mains ), | ||||||
| static fn( $p ) => '' !== $p && is_file( $p ) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also check for is_readable() following pcg_maybe_handle_probe()
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 39711d8 — added is_readable($p) to the up-front filter. |
||||||
| ) | ||||||
| ); | ||||||
| if ( empty( $plugin_mains ) ) { | ||||||
| return array( | ||||||
| 'status' => 'error', | ||||||
| 'reason' => 'Plugin main file not found for load probe.', | ||||||
| 'reason' => 'No probable plugin main files supplied.', | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| $front = $this->prepare_probe( $plugin_main, home_url( '/' ), false, $mode ); | ||||||
| $admin = $this->prepare_probe( $plugin_main, admin_url( 'index.php' ), true, $mode ); | ||||||
| $front = $this->prepare_probe( $plugin_mains, home_url( '/' ), false, $mode ); | ||||||
| $admin = $this->prepare_probe( $plugin_mains, admin_url( 'index.php' ), true, $mode ); | ||||||
|
|
||||||
| try { | ||||||
| $responses = \WpOrg\Requests\Requests::request_multiple( | ||||||
|
|
@@ -75,8 +73,8 @@ public function test( $plugin_main, $mode = self::MODE_ACTIVATION ) { | |||||
| delete_transient( self::transient_key( $admin['token'] ) ); | ||||||
| } | ||||||
|
|
||||||
| $front_result = $this->parse_response( $responses['front'], $plugin_main, false ); | ||||||
| $admin_result = $this->parse_response( $responses['admin'], $plugin_main, true ); | ||||||
| $front_result = $this->parse_response( $responses['front'], false ); | ||||||
| $admin_result = $this->parse_response( $responses['admin'], true ); | ||||||
|
|
||||||
| // fatal/throwable wins; an inconclusive `error` from one probe must | ||||||
| // not shadow a real fatal from the other. Front-end is the canonical | ||||||
|
|
@@ -108,30 +106,30 @@ protected function is_block( $result ) { | |||||
| * needing a live HTTP loopback. Not part of the public API. | ||||||
| * | ||||||
| * @internal | ||||||
| * @param string $plugin_main Absolute path to the plugin's main PHP file. | ||||||
| * @param string $mode Probe mode constant. | ||||||
| * @return array{plugin:string,mode:string} | ||||||
| * @param string[] $plugin_mains Absolute paths to plugin main PHP files. | ||||||
| * @param string $mode Probe mode constant. | ||||||
| * @return array{plugins:string[],mode:string} | ||||||
| */ | ||||||
| public static function build_probe_payload( $plugin_main, $mode = self::MODE_ACTIVATION ) { | ||||||
| public static function build_probe_payload( array $plugin_mains, $mode = self::MODE_ACTIVATION ) { | ||||||
| return array( | ||||||
| 'plugin' => (string) $plugin_main, | ||||||
| 'mode' => self::MODE_UPDATE === $mode ? self::MODE_UPDATE : self::MODE_ACTIVATION, | ||||||
| 'plugins' => array_values( array_map( static fn( $p ) => (string) $p, $plugin_mains ) ), | ||||||
| 'mode' => self::MODE_UPDATE === $mode ? self::MODE_UPDATE : self::MODE_ACTIVATION, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Stash a probe transient and build the request descriptor for | ||||||
| * `Requests::request_multiple`. | ||||||
| * Stash a probe transient and build the `Requests::request_multiple` | ||||||
| * descriptor for one of the two parallel probes. | ||||||
| * | ||||||
| * @param string $plugin_main Absolute path to the plugin's main PHP file. | ||||||
| * @param string $base_url Base URL to probe (front-end or admin). | ||||||
| * @param bool $is_admin Adds `pcg_admin=1` and forwards auth cookies. | ||||||
| * @param string $mode Probe mode constant. | ||||||
| * @param string[] $plugin_mains Absolute paths to plugin main PHP files. | ||||||
| * @param string $base_url Front-end or admin base URL. | ||||||
| * @param bool $is_admin Adds `pcg_admin=1` and forwards auth cookies. | ||||||
| * @param string $mode Probe mode constant. | ||||||
| * @return array{token:string,request:array} | ||||||
| */ | ||||||
| protected function prepare_probe( $plugin_main, $base_url, $is_admin, $mode = self::MODE_ACTIVATION ) { | ||||||
| protected function prepare_probe( array $plugin_mains, $base_url, $is_admin, $mode = self::MODE_ACTIVATION ) { | ||||||
| $token = wp_generate_password( 32, false ); | ||||||
| set_transient( self::transient_key( $token ), self::build_probe_payload( $plugin_main, $mode ), self::TOKEN_LIFETIME ); | ||||||
| set_transient( self::transient_key( $token ), self::build_probe_payload( $plugin_mains, $mode ), self::TOKEN_LIFETIME ); | ||||||
|
|
||||||
| $query = array( | ||||||
| 'pcg_probe' => '1', | ||||||
|
|
@@ -159,13 +157,12 @@ protected function prepare_probe( $plugin_main, $base_url, $is_admin, $mode = se | |||||
| /** | ||||||
| * Translate a `Requests::request_multiple` response into a probe verdict. | ||||||
| * | ||||||
| * @param mixed $response A `WpOrg\Requests\Response`, or an exception | ||||||
| * thrown for that single request. | ||||||
| * @param string $plugin_main Plugin main file (for fallback diagnostics). | ||||||
| * @param bool $is_admin True when this was the admin probe. | ||||||
| * @return array{status:string,reason?:string,errno?:int,class?:string,message?:string,file?:string,line?:int} | ||||||
| * @param mixed $response A `WpOrg\Requests\Response`, or an exception | ||||||
| * thrown for that single request. | ||||||
| * @param bool $is_admin True when this was the admin probe. | ||||||
| * @return array{status:string,reason?:string,errno?:int,class?:string,message?:string,file?:string,line?:int,plugin?:string} | ||||||
| */ | ||||||
| protected function parse_response( $response, $plugin_main, $is_admin ) { | ||||||
| protected function parse_response( $response, $is_admin ) { | ||||||
| if ( $response instanceof \Throwable ) { | ||||||
| return array( | ||||||
| 'status' => 'error', | ||||||
|
|
@@ -194,8 +191,6 @@ protected function parse_response( $response, $plugin_main, $is_admin ) { | |||||
| return array( | ||||||
| 'status' => 'fatal', | ||||||
| 'message' => 'Probe request returned HTTP 500 without a JSON verdict; the plugin likely fatals during load.', | ||||||
| 'file' => basename( $plugin_main ), | ||||||
| 'line' => 0, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -206,11 +201,9 @@ protected function parse_response( $response, $plugin_main, $is_admin ) { | |||||
| return array( | ||||||
| 'status' => 'fatal', | ||||||
| 'message' => sprintf( | ||||||
| 'Probe completed without a verdict (HTTP %d, non-JSON body). The plugin may have terminated the request during load, init, or admin_init.', | ||||||
| 'Probe completed without a verdict (HTTP %d, non-JSON body). A plugin in the batch may have terminated the request during load, init, or admin_init.', | ||||||
| $code | ||||||
| ), | ||||||
| 'file' => basename( $plugin_main ), | ||||||
| 'line' => 0, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In cases where the $result payload doesn't contain
pluginorfile(for instance, https://github.com/Automattic/jetpack/pull/48402/changes#diff-babcd1ba3a7b2e3b238816b14f26e591dfd6bfed2dcb3578bb05b6b0ceb4eacaR190-R195), then returning the first plugin might lead to wrong attribution. The batch would still be blocked, but the notice might point to an incorrect plugin.Maybe we can have a batch-level attribution, something like "One of these plugins caused a fatal error during the pre-flight check: A, B, C. Investigate before trying again.".
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.
Done in 59f6cc9 —
pcg_guard_get_blocked_pluginreturns''when attribution can't be pinned; caller emits a batch-level notice line: "One of these plugins caused a fatal during the pre-flight check: A, B, C. Reason: …". Renderer drops the<code>plugin</code>prefix when the key is empty.