chore: drop external payload not found errors#4308
Open
grutt wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Benchmark resultsCompared against |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent RabbitMQ pre-ack retry storms by treating “external payload not found” as a terminal (permanent) pre-ack failure and enriching drop logs with structured context for alerting/filtering.
Changes:
- Introduces a shared
ErrExternalPayloadNotFoundsentinel andExternalPayloadNotFoundErrorwrapper in the repository layer. - Extends RabbitMQ pre-ack error classification to treat
ErrExternalPayloadNotFoundas permanent and logspermanent_pre_ack_reasonplus external payload metadata. - Adds unit coverage for the new permanent pre-ack classification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/repository/payloadstore.go | Adds a typed/sentinel error to represent terminal external payload misses. |
| internal/msgqueue/rabbitmq/rabbitmq.go | Classifies external-payload-not-found as a permanent pre-ack failure and enriches drop logs with structured fields. |
| internal/msgqueue/rabbitmq/permanent_preack_test.go | Adds test coverage for the external-payload-not-found permanent classification. |
Comment on lines
926
to
928
| if errors.Is(err, repository.ErrExternalPayloadNotFound) { | ||
| return "external_payload_not_found" | ||
| } |
abelanger5
reviewed
Jun 29, 2026
| return permanentPreAckErrorReason(err) != "" | ||
| } | ||
|
|
||
| func permanentPreAckErrorReason(err error) string { |
Contributor
There was a problem hiding this comment.
I don't think this can work at the msgqueue layer, because messages are buffered (on both the pub and sub side). So consumers will need to shed these messages.
If we wanted to only nack part of the batch we'd need need to support individual acks for each message in the buffer, which we don't currently. But even if we did this, there's no way of getting around the pub buffer (which is why the pub buffer should be used sparingly)
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
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.
Description
Adds a terminal external payload not found error and teaches RabbitMQ pre-ack handling to drop those messages instead of retrying indefinitely. This prevents missing S3 offloaded payload index files from generating noisy retry storms, while keeping other S3 failures retryable.
The permanent drop log is enriched with structured fields like
permanent_pre_ack_reason=external_payload_not_found,external_payload_kind, andexternal_payload_keyfor filtering and alerting.Fixes #
Type of change
What's Changed
ErrExternalPayloadNotFoundtyped error for terminal external payload misses.NoSuchKeyindex file fetch errors to the terminal payload error.Checklist
Changes have been:
Testing
go test ./internal/msgqueue/rabbitmq -run 'TestIsPermanentPreAckError' -count=1go test ./pkg/repository -run '^$' -count=1GOWORK=<temp local workspace> go test github.com/hatchet-dev/hatchet-cloud/internal/repository -run 'Test(FetchWithFallback|IndexFileFetchError)' -count=1🤖 AI Disclosure
I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchet's AI_POLICY.md.
Details: Cursor agent was used to inspect the retry/error paths, implement the targeted code changes, add focused tests, and run validation commands.