fix: site deregistration in both brand and governing site - #306
Draft
Kallyan01 wants to merge 1 commit into
Draft
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical concurrency flaw and unresolved remote-failure and retry-authentication issues can leave pairings inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds symmetric deregistration between governing and brand sites through an authenticated REST endpoint.
Changes:
- Propagates disconnects across paired sites.
- Clears cached configuration and reports partial failures.
- Adds PHP and React integration coverage.
File summaries
| File | Review |
|---|---|
tests/phpunit/Integration/Modules/Settings/SettingsTest.php |
Tests removed-brand notifications. |
tests/phpunit/Integration/Modules/Rest/Governing_Data_HandlerTest.php |
Tests disconnect request handling. |
tests/phpunit/Integration/Modules/Rest/Governing_Data_Controller_GoverningSiteTest.php |
Tests governing-site endpoint behavior. |
tests/phpunit/Integration/Modules/Rest/Governing_Data_Controller_BrandSiteTest.php |
Tests brand-site endpoint behavior. |
tests/phpunit/Integration/Modules/Rest/Basic_Options_ControllerTest.php |
Tests local and remote disconnect outcomes. |
tests/js/SiteSettings.test.tsx |
Tests success and warning notices. |
inc/Modules/Settings/Settings.php |
Detects removed brand sites and initiates notifications. |
inc/Modules/Rest/Governing_Data_Handler.php |
Moderate: Governing-side notifications discard transport errors and non-200 responses, leaving stale remote state while reporting success. Preserve failures for retry or expose their status. |
inc/Modules/Rest/Governing_Data_Controller.php |
Critical: Concurrent deregistrations can overwrite each other and resurrect removed sites; serialize updates or use compare-and-swap retries. Moderate: Idempotent retries receive 403 because authentication state is already removed; retain authenticated tombstones or remove the guarantee and adjust retry handling/tests. |
inc/Modules/Rest/Basic_Options_Controller.php |
Propagates brand-initiated disconnects and reports remote failures. |
inc/Modules/Rest/Abstract_REST_Controller.php |
Resolves requesting-site URLs. |
assets/src/components/SiteSettings.tsx |
Displays partial-failure warnings. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ); | ||
| } | ||
|
|
||
| $shared_sites = Settings::get_shared_sites(); |
Comment on lines
+177
to
+179
| // Already gone: nothing to do, but the caller got what it asked for. | ||
| if ( ! isset( $shared_sites[ $site_url ] ) ) { | ||
| return rest_ensure_response( |
| continue; | ||
| } | ||
|
|
||
| self::request_disconnect( $site_url, $api_key ); |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
============================================
+ Coverage 85.95% 86.47% +0.51%
- Complexity 613 646 +33
============================================
Files 22 22
Lines 2158 2284 +126
============================================
+ Hits 1855 1975 +120
- Misses 303 309 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Kallyan01
marked this pull request as draft
September 4, 2026 17:24
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Makes disconnecting a governing/brand pair take effect on both sites instead of
only the one the action was performed on.
Why
A pairing lives in two options that never spoke to each other —
onesearch_shared_siteson the governing site,onesearch_parent_site_urlon thebrand site — so either side could tear it down and leave the other believing the
pair was still connected.
Brand side (the reported bug).
Disconnect Governing Sitedeleted the localoption and stopped there. The governing site kept listing the brand along with its
stored API key, indexable entities and indexed records.
Governing side. Deleting a brand row cleaned up the governing end properly, but
nothing told the brand. It kept showing the governing site URL under Governing
Site Connection and kept serving its cached brand config for up to a week.
So the pairing was one-sided in both directions, just in opposite ways.
Related Issue(s):
How
A single symmetric endpoint,
DELETE /onesearch/v1/connection, registered on bothsite types behind the existing
check_api_permissionsguard.AI Disclosure
Claude Code (Opus 5), used for diagnosis, implementation, and tests.
Testing Instructions
Needs two paired installs — e.g.
npm run wp-env start(governing,:8888) andnpm run wp-env:child start(brand,:8890).Confirm the brand appears under Brand Sites, and that the brand site shows the
governing URL under Governing Site Connection.
notice appears and the field clears.
list. On
mainit would still be there.the disconnect button is disabled. On
mainit would still show the governing site.onesearch_parent_site_urlat an unreachable host) and disconnect from the brandside. The brand still disconnects locally, but the notice is a warning saying
the governing site could not be notified.
Checklist