Skip to content

[Fix] PR review feedback disappears during pull request creation - #1262

Closed
roomote-roomote[bot] wants to merge 9 commits into
developfrom
fix/pr-association-race-2g24z2346jz46
Closed

[Fix] PR review feedback disappears during pull request creation#1262
roomote-roomote[bot] wants to merge 9 commits into
developfrom
fix/pr-association-race-2g24z2346jz46

Conversation

@roomote-roomote

@roomote-roomote roomote-roomote Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

​Opened on behalf of @daniel-lxs. View the task or mention @roomote-roomote for follow-up asks.

What changed

  • Retain unlinked review events in one provider-isolated Redis orphan list and one bounded five-attempt BullMQ replay chain per pull request.
  • Preserve the existing GitHub pending and scheduled Redis key identity across rolling deploys while namespacing non-default source-control providers.
  • Release only replay ownership at the final no-association boundary; the retained list remains available for TTL cleanup, a later webhook, or association-commit wake.
  • Persist a TTL-bound repair intent before every orphan replay enqueue and scan at most 100 due intents each minute.
  • Make stale repair cleanup atomic: invalid payloads are removed only when their exact raw value still matches, while missing-payload index entries are removed only while the payload key remains absent.
  • Preserve newer concurrent repair intents across stale sweeps and repeated queue failures so they can recover without another webhook.
  • Carry the source-control provider into BullMQ pending-event drains.
  • Preserve the Redis endpoint supplied by CI in the real-Redis test harness, using port 16379 only when REDIS_URL is unset locally.
  • Pass REDIS_URL through Turborepo's strict environment allowlist so the CI test task reaches its Redis service on port 6379.

Why this change was made

Review webhooks can arrive before Roomote commits the task-to-pull-request association. Repair intent cleanup must not delete a newer intent recorded concurrently after a stale read, or retained feedback can again become stranded until expiry.

The real-Redis test previously replaced the CI-provided Redis endpoint with a local-only port, and Turborepo then filtered the corrected endpoint before starting the SDK test. The test therefore timed out against port 16379 in GitHub Actions even though CI's Redis service was healthy on port 6379.

PR #1265 remains an unmerged draft and repairs task-level pending notification jobs. This PR uses a separate orphan-association replay repair namespace and has no dependency on #1265.

Impact

Review feedback arriving concurrently with pull-request creation can recover after repeated replay enqueue outages without another webhook, including when a stale repair sweep races with a newer intent. The repair worker remains bounded to 100 due intents per minute and the existing 15-minute orphan TTL. CI now preserves its configured Redis endpoint through the Turbo test boundary, while local runs retain the port 16379 fallback. There is no product or runtime behavior change from the CI configuration fix.

@roomote-community

roomote-community Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

1 issue outstanding. See task

  • apps/bullmq/src/jobs/pr-review-notification.ts:237 Provider-qualified pending events are drained through the matching provider key.
  • packages/sdk/src/server/lib/task-runs/pr-review-notification.ts:1082 A double replay-enqueue failure after association commit can still strand retained orphan feedback until TTL expiry.
  • packages/sdk/src/server/lib/task-runs/pr-review-notification.ts:557 A repeatedly failing full repair batch starves every due repair intent after its first 100 entries.

Reviewed 510135a

@roomote-roomote

roomote-roomote Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the stale repair cleanup race in commit e7dfd55.

  • Invalid non-null payload cleanup now atomically deletes the payload and index member only when the exact stale raw value still matches.
  • Missing-payload cleanup now atomically removes only the index member while the payload key remains absent; it never deletes a concurrently written value.
  • Existing valid-payload compare-and-clear behavior is unchanged.
  • Real-Redis regressions inject a newer valid intent after both missing and invalid stale reads, fail the newer enqueue once, and prove its payload/index survive and later recover without another webhook.

Passed focused SDK tests (84), BullMQ tests (31), GitHub webhook tests (53), SDK/BullMQ/API typechecks, formatting, and repository pre-push checks. Visual proof is not applicable to this Redis/Lua concurrency behavior.

Comment thread packages/sdk/src/server/lib/task-runs/pr-review-notification.ts
Comment thread packages/sdk/src/server/lib/task-runs/pr-review-notification.ts Outdated
now = Date.now(),
}: { now?: number } = {}): Promise<void> {
const redis = getRedis();
const members = await redis.zrangebyscore(

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.

The bounded scan repeatedly reads the same first 100 overdue members. If Queue.add remains down for all of them, this catch leaves their unchanged scores in the ZSET; each one-minute repair run selects those same members again and never reaches later due repair intents. A busy outage with over 100 affected PRs therefore leaves later retained feedback unscheduled until its 15-minute TTL expires. Reschedule failed entries (or use a cursor) so a failed batch cannot starve the rest of the index.

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.

This remains unresolved in e7dfd55: the Queue.add failure path only logs and leaves the member's score unchanged. Every subsequent ZRANGEBYSCORE ... LIMIT 0 100 therefore selects the same failed entries before later due intents, so an outage affecting more than 100 PRs can still leave the later orphan lists unscheduled until their 15-minute TTL expires. Reschedule failed members to a future score (or advance a persisted cursor) before returning.

@daniel-lxs

Copy link
Copy Markdown
Member

Closing in favor of a single Postgres-backed durable event/outbox design. This branch’s Redis replay ownership overlaps with queue-recovery behavior, which can give two mechanisms ownership of the same failed delivery. That is not an invariant we can prove or an architecture we want to maintain.

@daniel-lxs daniel-lxs closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant