[Fix] PR feedback waits for scheduled recovery after triage failures - #2272
Conversation
|
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. |
Related issue
No new issue. Scoped internal PR-feedback reliability improvement.
Why this PR exists
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_URLpassthrough in Turbo. CI provides Redis on 6379, but the stripped variable caused tests to fall back to 16379. Added the variable alongsideDATABASE_URLinglobalPassThroughEnv; 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.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.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.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
[Fix],[Feat],[Improve],[Refactor],[Docs], or[Chore]followed by a user-facing descriptionpnpm lintandpnpm check-typespass locallypnpm changesetThe full formatting-inclusive lint and tsc monorepo commands were not run; package tsc and pre-push fast gates passed. No changeset added.