Skip to content

[Fix] PR feedback waits for scheduled recovery after triage failures - #2272

Merged
roomote-roomote[bot] merged 2 commits into
developfrom
fix/pr-feedback-preparation-retry-2b5crtokhf5ks
Sep 6, 2026
Merged

[Fix] PR feedback waits for scheduled recovery after triage failures#2272
roomote-roomote[bot] merged 2 commits into
developfrom
fix/pr-feedback-preparation-retry-2b5crtokhf5ks

Conversation

@roomote-roomote

@roomote-roomote roomote-roomote Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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

Related issue

No new issue. Scoped internal PR-feedback reliability improvement.

Why this PR exists

  • A maintainer explicitly invited this PR in the linked issue or discussion
  • I am a maintainer / this is internal Roomote work

When PR-feedback triage fails, the notification handler clears its database lease before throwing. BullMQ's configured retry then reuses the invalid token and skips as superseded. Feedback waits for the minute recovery scheduler rather than getting its intended short retry.

What changed

Keep the canonical claimed lease across preparation failures only while BullMQ has attempts remaining. The next queue attempt can renew the same claim and retry preparation using the existing 2-second exponential backoff. No new retry budget or scheduler is introduced.

Rate limits still defer explicitly. Exhausted attempts, failures after preparation, legacy delivery ownership, and resumed non-claimed states keep the existing release/recovery path. Posting and automatic action semantics are unchanged.

This removes avoidable scheduler waiting from this failure mode; it does not speed up the inference provider, avoid repeated preparation, or promise a fixed end-to-end delivery time. If the queue disappears while a retry is pending, the existing ten-minute lease expiry remains the durable recovery backstop.

How it was tested

Evidence classification: withheld pending external-route proof. GitHub PR state is non-draft and is preserved. CI run 33990764625 passed on exact head 410f5c998747bb91ecc875880bfb978e4beee3ac, and the synchronized code review reports no new issues on that SHA, with no unresolved threads. The actual queue/database retry path is verified; full webhook-to-Session delivery and production latency improvement remain unverified. A signed webhook and authenticated GitHub installation test fixture are still needed; no mocked route is being presented as external proof.

CI follow-up: the new queue test exposed missing REDIS_URL passthrough in Turbo. CI provides Redis on 6379, but the stripped variable caused tests to fall back to 16379. Added the variable alongside DATABASE_URL in globalPassThroughEnv; no test was skipped and no runtime retry behavior changed. Forced validation through Turbo passed all 505 BullMQ tests across 43 files: REDIS_URL=redis://127.0.0.1:16379 pnpm exec turbo run test --filter=@roomote/bullmq --force -- --silent. Dry-run output confirms the configured Redis URL is passed through. Fresh CI, including Test, passed on the corrective commit.

  • Before the fix, two new unit regressions failed because the first and second queue attempts released their claims.
  • 60 tests passed: pnpm exec dotenvx run -f .env.test -- pnpm --filter @roomote/bullmq exec vitest run src/jobs/pr-review-notification.test.ts src/jobs/pr-review-notification-retry.db.test.ts src/jobs/pr-review-notification.db.test.ts --silent.
  • The new real PostgreSQL/Redis test persists and claims a canonical event, executes the real handler with a BullMQ Worker, fails preparation once, and completes on the queue retry without a scheduler drain. A second case invalidates the lease between attempts and confirms the stale retry does not prepare again. The isolated test queue is removed afterward.
  • Simulation disclosure: preparation/triage is mocked to fail once and then suppress; the database, lease renewal/release, BullMQ execution and 2-second backoff are real. No GitHub webhook, inference request or chat notification is sent. This proves retry ownership, not external presentation or model timing.
  • Package pnpm --filter @roomote/bullmq check-types, changed-file oxfmt, and diff checks passed. Pre-push oxlint, residual ESLint, repository fast typechecks and knip passed. Knip emitted a non-failing docs mint configuration hint.
  • Browser proof is not applicable to this backend retry lifecycle. No screenshots or videos are claimed.

Before changing readiness classification, verify the full webhook-to-Session path on this head with a controlled first triage failure and inspect final CI/review results. Existing PR #2149 changes later parent-event admission in the same handler; its inspected patch does not overlap this preparation/catch change, but integration should be rechecked if it lands first.

Checklist

  • The PR title follows the repo convention: [Fix], [Feat], [Improve], [Refactor], [Docs], or [Chore] followed by a user-facing description
  • This PR is small and scoped to one change
  • pnpm lint and pnpm check-types pass locally
  • I added tests or included a clear manual validation note above
  • I removed secrets, tokens, private keys, and customer data from code, logs, and screenshots
  • If this change should appear in the changelog, I ran pnpm changeset

The full formatting-inclusive lint and tsc monorepo commands were not run; package tsc and pre-push fast gates passed. No changeset added.

@roomote-community

roomote-community Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

No new code issues found. See task

Reviewed 410f5c9

@roomote-roomote
roomote-roomote Bot marked this pull request as ready for review September 5, 2026 20:30
@roomote-roomote

roomote-roomote Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The CI failure is fixed in 410f5c99: Turbo now passes CI's REDIS_URL through instead of letting the new queue tests use the wrong fallback port. No tests were skipped or retry behavior changed.

CI run 33990764625, including Test, passed on that exact head. The synchronized review reports no new code issues on the same SHA, with no unresolved review threads. Local forced Turbo validation passed all 505 BullMQ tests.

Evidence readiness remains withheld for full webhook-to-Session proof. The real queue/database retry leg is covered, but an authenticated signed-webhook/GitHub installation test fixture is still required for the external route. No external-route success or production latency reduction is claimed. Existing non-draft PR state is preserved. PR #2270 was not modified; nothing was merged or deployed.

@roomote-roomote
roomote-roomote Bot merged commit 316e98d into develop Sep 6, 2026
17 checks passed
@roomote-roomote
roomote-roomote Bot deleted the fix/pr-feedback-preparation-retry-2b5crtokhf5ks branch September 6, 2026 14:40
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