Continue reaction sync past the page cap and recover missed replies

[pfefferle]

Proposed changes:

• Resume pagination across runs. Reaction sync previously advanced its watermark after five pages (250 notifications) per run, permanently stranding every older reply and reaction on busy accounts. The continuation cursor is now persisted per stream and resumed on the next run, with a one-time replay of the completed range so nested replies processed in an earlier chunk than their parent still thread correctly. Repeated continuation failures (3×) discard the cursor and restart from the newest page.
• Daily rolling reply audit. A new daily cron checks a small batch of published posts (default 5, filterable via atmosphere_reply_backfill_batch_size, 0 disables) against Bluesky's public getPostThread and imports replies that are missing locally — recovering replies that were already stranded before this fix. Never-checked posts go first; afterwards the least-recently checked posts rotate through.
• On-demand recovery via WP-CLI. wp atmosphere replies backfill <post-id> scans one post's thread immediately and reports found / imported / already present / skipped / pending counts.
• Shared lease lock. The hourly sync and both backfill paths coordinate through a renewable cross-process lease lock (new reusable Atmosphere\Lock class: atomic options-table acquire, owner token, CAS steal of expired rows, renewal throttled to fire only when less than half the TTL remains) so they never run concurrently. OAuth\Client's older lock can migrate to it in a follow-up.
• Moderation-safe dedup. The source-URI dedup lookups now include spammed/trashed comments, so a reply the admin moderated away is not re-imported by a later audit.
• Thread imports are capped at 500 reply nodes per run, and the daily audit is scheduled offset by 30 minutes so its due timestamps never collide with the hourly sync's.

Other information:

• Have you written new tests for your changes, if applicable?

Testing instructions:

• npm run env-start, connect the plugin to a Bluesky account, and publish a post.
• Reply to the post on Bluesky (add a nested reply to your reply as well), then run wp-env run cli wp atmosphere replies backfill <post-id>: both replies should appear as threaded comments and the summary should report imported: 2. Run it again — already present: 2, no duplicates.
• Mark one imported reply as spam in wp-admin and re-run the command: the spammed reply must not come back (already present still counts it).
• For the overflow fix: npm run env-test -- --filter=paginate covers watermark, continuation, replay, and the page-budget boundary cases; npm run env-test -- --filter='backfill|replies|lock' covers the audit, CLI command, and lock semantics.
• Full suite: composer lint and npm run env-test (929 tests passing).

[Copilot]

Pull request overview

This PR improves reply/reaction ingestion reliability by continuing reaction-sync pagination across cron runs, adding mechanisms to recover previously missed replies, and preventing concurrent sync/backfill workers via a shared cross-process lease lock.

Changes:

• Persist and resume reaction-sync pagination state (with replay to recover cross-chunk parent/child threading), and reset state safely on DID change or repeated cursor failures.
• Add reply recovery paths: daily rolling reply audit via WP-Cron and an on-demand WP-CLI command to backfill a single post’s replies via Bluesky’s public getPostThread.
• Introduce a reusable options-table lease lock (Atmosphere\Lock) shared across reaction sync and reply backfills; expand uninstall cleanup and update docs/changelog/tests accordingly.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
uninstall.php	Cleans up new pagination/DID state, lock option, and reply-backfill meta on uninstall.
tests/phpunit/tests/cli/class-test-replies-command.php	Adds WP-CLI coverage for the replies backfill command behavior and reporting.
tests/phpunit/tests/class-test-reaction-sync.php	Adds/updates tests for pagination continuation/replay, DID resets, lock semantics, and reply backfill helpers.
tests/phpunit/tests/class-test-atmosphere.php	Ensures the new daily backfill cron hook is scheduled and cleaned up in tests.
readme.txt	Documents automatic rolling reply recovery and the new WP-CLI command.
includes/functions.php	Registers the new atmosphere_backfill_replies hook in the canonical cron-hook list.
includes/cli/class-replies-command.php	Implements wp atmosphere replies backfill <post-id> command.
includes/class-reaction-sync.php	Implements pagination continuation/replay, scheduled reply audit, thread import + dedup changes, and shared-lock integration.
includes/class-lock.php	Adds an options-table-backed lease lock with TTL, renew, and safe release semantics.
includes/class-cli.php	Registers the new atmosphere replies WP-CLI command group.
includes/class-atmosphere.php	Wires the new daily reply-backfill cron event (offset) and handler registration.
docs/php-coding-standards.md	Documents the new atmosphere_reply_backfill_batch_size filter.
.github/changelog/fix-reaction-pagination-backfill	Adds a patch-level changelog entry describing pagination + reply recovery improvements.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

[jeherve]

A couple of behavior questions from the review

1. resolve_author() when getProfile fails (class-reaction-sync.php:1683)

I noticed resolve_author() returns array() on a getProfile WP_Error, and it's the only is_wp_error branch in the file that returns without a debug_log. The callers (process_reply, process_subject_reaction, process_own_record) then insert the comment anyway with an empty name/handle/avatar.

The reason I'm flagging this one over a normal transient blip: the success transient is only written after getProfile succeeds (:1693), so a failure isn't cached, but the comment still gets imported. Once the watermark moves past it and source_id dedup kicks in, it never gets revisited. So a brief rate-limit or 5xx ends up baked in as a permanent comment with a missing avatar, and for own-record reactions an empty handle, which gives a broken comment_author_url. The daily reply-backfill re-audits replies but not likes/reposts, so those don't self-heal either.

I'm wondering if importing-with-empty-profile is the degradation you actually want here, or whether a getProfile failure should defer that reaction so a later run retries it. And either way, could we add a debug_log on that branch? It's the one failure in the file that leaves no trace today, so right now we'd have no way to spot it in the field. What do you think?

2. DEDUP_COMMENT_STATUSES and parent resolution (class-reaction-sync.php:150, used at :1327)

The widened status set (adding spam/trash/post-trashed) makes total sense for the dedup lookup, and the docblock explains it well; without it the backfill re-imports a reply the admin moderated away on every audit. No concern there.

The bit I want to double-check is intent. find_comment_by_source_id() is also the parent-resolution lookup in process_reply() (:1327), so it inherits the wider set too. Before this change, a reply whose parent had been spammed or trashed would fail to resolve its parent and get dropped as an orphan at :1360. After it, that parent resolves, so the child gets imported and attached to a comment the admin had removed; and if the child passes its own moderation, it shows up as a visible sub-thread hanging off a suppressed parent.

Is that intended? The :1339 rationale is all about orphan and out-of-order handling and doesn't mention moderated parents, so it reads to me like an incidental side effect of one lookup doing two jobs. If we'd rather not resurface under a moderated parent, one option is to keep dedup on the wide set but resolve parents only among approve/hold with a second, narrower lookup for the :1327 call. Curious what you were picturing here.

[pfefferle]

Both good catches — thanks for the careful read. Addressed in 0a1c2ac:

1. resolve_author() failures — added the missing debug_log on that branch (you're right that it was the only untraced failure in the file). One nuance: for notification-stream and thread-backfill reactions, insert_reaction() already falls back to the payload's own author.handle, so a getProfile blip there costs only the display name and avatar — not the author URL. The genuinely broken case was own-record reactions, where the handle itself came from getProfile; those now fall back to the locally stored identity handle, so the author link can't break at all. On defer-vs-import: I kept import-with-degraded-profile deliberately — a permanently deleted account's getProfile fails forever, so deferral would mean those reactions never import, and the reaction content matters more than its avatar. With the fallbacks, the permanent loss is displayName/avatar only, now with a breadcrumb.

2. Moderated parents — you read it exactly right: the widening was meant for dedup and leaked into parent resolution as a side effect of one lookup doing two jobs. The orphan rationale at the call site even lists "removed from our moderation queue" as a drop case, so this was a regression of documented intent. Fixed per your suggestion: find_comment_by_source_id() takes a status list now — dedup keeps the wide set, parent resolution passes approve/hold only — with a regression test (spammed parent → incoming child drops as an orphan).