fix: stabilize queued retry dispatch#2192
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors queue dispatch retry and distributed startup coordination. The retry command now looks up queued-dispatch targets and conditionally reuses queued attempts based on trigger type. The scheduler's startup logic shifts from error-only channels to execution-done callbacks, allowing startup checks to account for whether distributed execution has completed and handle completion as a terminal condition with explicit failure paths. ChangesQueue Dispatch Retry Enhancement
Distributed Execution Startup Coordination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cmd/retry_test.go (1)
234-237: ⚡ Quick winAssert the preserved trigger type in the reuse case.
This subtest proves the queued attempt ID is reused, but it would still pass if the execution path rewrote the reused run to
core.TriggerTypeRetry. Adding an assertion forlatestStatus.TriggerType == core.TriggerTypeWebhookwould lock in the behavior this change is preserving.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/retry_test.go` around lines 234 - 237, Add an assertion after reading latestStatus in the reuse-case subtest to ensure the preserved trigger type isn't overwritten: after calling latestAttempt.ReadStatus(th.Context) and the existing require.Equal for core.Succeeded, assert that latestStatus.TriggerType equals core.TriggerTypeWebhook (i.e., require.Equal(t, core.TriggerTypeWebhook, latestStatus.TriggerType)) so the test fails if the reused run is mistakenly changed to core.TriggerTypeRetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/retry.go`:
- Around line 177-185: The closure that calls queueDispatchRetryTarget must not
re-resolve ref and return a different attempt than the one whose status/dag were
previously loaded; update the code so it pins to the original attempt ID (the
previously loaded attempt variable) instead of reselecting by ref: either pass
the original attempt.ID into queueDispatchRetryTarget (or add an overload) and
have it validate/return only that attempt, or skip re-resolving and reuse the
already-loaded attempt/status/dag when constructing queuedAttempt; ensure
executeRetry/preparedAttempt and rootRun/trigger/log metadata remain paired with
the same pinned attempt ID and validate any mismatch if queueDispatchRetryTarget
must still requery.
---
Nitpick comments:
In `@internal/cmd/retry_test.go`:
- Around line 234-237: Add an assertion after reading latestStatus in the
reuse-case subtest to ensure the preserved trigger type isn't overwritten: after
calling latestAttempt.ReadStatus(th.Context) and the existing require.Equal for
core.Succeeded, assert that latestStatus.TriggerType equals
core.TriggerTypeWebhook (i.e., require.Equal(t, core.TriggerTypeWebhook,
latestStatus.TriggerType)) so the test fails if the reused run is mistakenly
changed to core.TriggerTypeRetry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e5fad76-e984-4765-9949-0fa8c1ee1c43
📒 Files selected for processing (6)
internal/cmd/retry.gointernal/cmd/retry_test.gointernal/service/chatbridge/monitor_test.gointernal/service/scheduler/queue_dispatcher.gointernal/service/scheduler/queue_processor.gointernal/service/scheduler/queue_processor_startup_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbf760d7a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Summary
Testing
Summary by CodeRabbit
Bug Fixes
Tests