refactor: move retry event data query into retry flow #654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
alexluong
force-pushed
the
model-delivery-event
branch
from
04c3ebd to
9277a3f
Compare
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extends TestRetryDelivery to verify that the manual retry API publishes a DeliveryTask with complete event data to deliveryMQ. This ensures the deliverymq handler receives full event data for manual retries, consistent with the scheduled retry flow which fetches event data in the retry scheduler. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add unit and e2e tests verifying that retries are not lost when the retry scheduler queries logstore before the event has been persisted. Test scenario: 1. Initial delivery fails, retry is scheduled 2. Retry scheduler queries logstore for event data 3. Event is not yet persisted (logmq batching delay) 4. Retry should remain in queue and be reprocessed later Tests added: - TestRetryScheduler_RaceCondition_EventNotYetPersisted (unit test) - TestE2E_Regression_RetryRaceCondition (e2e test) Also adds: - RetryVisibilityTimeoutSeconds config option - WithRetryVisibilityTimeout scheduler option - mockDelayedEventGetter for simulating delayed persistence Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Return error instead of nil so the retry message stays in queue and will be reprocessed after the visibility timeout. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
alexluong
force-pushed
the
deliverymq-logstore-deps
branch
from
eb51fdb to
b701223
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves event fetching from the delivery flow to the retry flow.
What changed
Why this design
Scheduled retry tasks only store event IDs (not full payloads) to keep queue messages small. Previously, the delivery handler had to detect retries via
event.Time.IsZero()and conditionally fetch event data.Moving this to the retry scheduler is cleaner because:
Both entry points now consistently provide full event data to the delivery queue: