Skip to content

Premium Analytics: harden sync stall detection against finished/milestone read gap - #49651

Closed
chihsuan wants to merge 1 commit into
trunkfrom
wooa7s-1546-harden-sync-stall-detection
Closed

Premium Analytics: harden sync stall detection against finished/milestone read gap#49651
chihsuan wants to merge 1 commit into
trunkfrom
wooa7s-1546-harden-sync-stall-detection

Conversation

@chihsuan

@chihsuan chihsuan commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Proposed changes

Defensive hardening from review feedback on #49267 (use-sync-status.ts). Not a live bug fix — the gap can't occur on the default full-sync path; this just removes a permanent failure mode for a theoretical edge case.

Why — useSyncStatus cleared polling permanently the first time isSyncStalled was true, with no tolerance. The one false-positive is a read gap: a poll seeing finished: true at 100% progress while the milestone write hasn't landed (initial_full_sync_finished === 0). The default path writes the milestone before finished, so this can't happen there; only the legacy queue-based Full_Sync leaves a sub-ms window. Unreachable in practice, but cheap to guard.

How — isSyncStalled now treats percentage >= 100 && milestone === 0 as finishing, not stalled. The hook already keeps polling for anything neither complete nor stalled, so it self-heals on the next tick. A genuine stall sits below 100%.

Related product discussion/links

Does this pull request change what data or activity we track or use?

No.

Testing instructions

  • cd projects/packages/premium-analytics && pnpm test packages/site-sync — all suites pass; pnpm run typecheck clean.
  • New tests cover the read gap (finished before milestone → keeps polling → completes) and that a genuine sub-100% stall still surfaces.

@chihsuan chihsuan self-assigned this Jun 16, 2026
@github-actions

github-actions Bot commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions Bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jun 16, 2026
@jp-launch-control

jp-launch-control Bot commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Code Coverage Summary

This PR did not change code coverage!

That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷

Full summary · PHP report

@chihsuan
chihsuan force-pushed the wooa7s-1546-harden-sync-stall-detection branch from b4948f7 to 0ab9f02 Compare June 16, 2026 05:48
@chihsuan chihsuan added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 16, 2026
@chihsuan
chihsuan requested a review from Copilot June 16, 2026 05:50
@chihsuan
chihsuan force-pushed the wooa7s-1546-harden-sync-stall-detection branch from 0ab9f02 to fa9898e Compare June 16, 2026 05:50
@chihsuan
chihsuan requested a review from a team June 16, 2026 05:50
Copilot AI reviewed Jun 16, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 hardens @automattic/jetpack-premium-analytics-site-sync sync stall detection to avoid a false “stalled” state when a poll observes finished/100% progress before the initial_full_sync_finished milestone becomes visible.

Changes:

  • Treat percentage >= 100 && initialFullSyncFinished === 0 as a “finishing” state (not stalled) so polling continues through a transient read gap.
  • Add unit and hook-level tests covering the read-gap case and confirming a real sub-100% stalled state still surfaces.
  • Add a patch changelog entry describing the defensive behavior change.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
projects/packages/premium-analytics/packages/site-sync/src/status.ts Adjusts stall detection to ignore the transient “100% but milestone still 0” finishing gap.
projects/packages/premium-analytics/packages/site-sync/src/status.test.ts Adds unit coverage for the finishing-gap behavior and confirms genuine stall behavior.
projects/packages/premium-analytics/packages/site-sync/src/hooks/tests/use-sync-status.test.ts Adds hook-level test to ensure polling continues through the finishing gap and then completes.
projects/packages/premium-analytics/changelog/wooa7s-1546-harden-sync-stall-detection Documents the defensive change in stall detection.

Comment thread projects/packages/premium-analytics/packages/site-sync/src/status.ts
Comment on lines +58 to 60
const isFinishing = status.percentage >= 100 && status.initialFullSyncFinished === 0;
return status.isStarted && ! status.isRunning && ! isSyncComplete( status ) && ! isFinishing;
}
@chihsuan

chihsuan commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Closing as won't-do. After investigation this isn't a real bug on the shipping path: the default Full_Sync_Immediately persists the milestone before finished, so no finished && milestone===0 window exists. The only theoretical gap is the legacy queue-based Full_Sync (not in the default module list) — a sub-ms race against a 3s poll. Hardening it properly requires a bounded grace period (predicate + counter + tests), which is disproportionate surface for an unreachable state (YAGNI). The investigation is preserved on WOOA7S-1546.

@chihsuan chihsuan closed this Jun 16, 2026
@github-actions github-actions Bot removed the [Status] Needs Review This PR is ready for review. label Jun 16, 2026
@chihsuan
chihsuan deleted the wooa7s-1546-harden-sync-stall-detection branch June 16, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

@chihsuan chihsuan

Labels

[Package] Premium Analytics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chihsuan