feat: attachment skip list for failing uploads#228
Open
gasperzgonec wants to merge 7 commits into
Open
Conversation
Attachments that deterministically fail with ECONNABORTED or a 5xx from the SDK-owned upload path were being retried from scratch on every lambda invocation forever, since lastProcessedAttachmentsIdsList only tracks successes. Track per-attachment transient failure counts across invocations and skip an attachment once it exceeds the configurable maxAttachmentFailures (default 3) WorkerAdapterOption. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t classification Connector-owned stream() callbacks (e.g. fetching from the external system) had no way to report a status code, so their errors could never be classified as transient like the SDK-owned upload path already is. Add an optional statusCode field to ExternalSystemAttachmentStreamingResponse.error and treat 5xx as transient in processAttachment, so connector-reported failures count toward the same maxAttachmentFailures skip-list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…heck
The backwards-compatibility test compared interface/class property types
by exact string equality, so widening `ErrorRecord` to
`ErrorRecord & { statusCode?: number }` (an additive, non-breaking change
since the new field is optional) was flagged as a breaking change.
Hoist the existing object-type compatibility checker to module scope and
extend it to also handle intersection types: a new intersection member is
accepted as long as it only adds optional properties. Apply the same
check to interface and class property types instead of exact string
equality.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ot across Previously, transient upload failures (ECONNABORTED, 5xx) were counted across invocations before an attachment was marked permanently failed: each invocation retried once, incremented a persisted counter, and gave up only after maxAttachmentFailures separate invocations. That meant an attachment could go 2 invocations without a real retry attempt beyond the one HTTP-layer retry axios-retry already does. Move the retry loop inside a single invocation instead: an attachment is retried immediately up to maxAttachmentFailures times in the same processAttachmentWithRetries call, and only added to failedAttachmentsIdsList (skipped in subsequent invocations) once it exhausts that budget within one invocation. If a timeout interrupts a retry sequence, the attachment is left unmarked so it's retried fresh next invocation rather than losing its progress. This still prevents the infinite-loop failure mode (deterministic failures retried forever across invocations) while giving genuinely intermittent failures several real attempts before giving up, without persisting a failure count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The stream function's HTTP client (axiosClient) already retries transient errors (ECONNABORTED, 5xx) with exponential backoff before returning one to processAttachment. Adding a second retry loop on top of that (from the previous commit) meant re-attempting a request whose retry budget was already exhausted at the HTTP layer, effectively multiplying an already-failed wait instead of giving up sooner. Mark an attachment permanently failed on the first transient error observed here, removing the in-invocation retry loop, maxAttachmentFailures option, and its default constant along with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add the attachment's fetch URL and a safe allowlist of response headers (content-length, content-type, content-disposition, content-encoding, last-modified, etag, cache-control) to every "Skipping attachment" log line, to make it easier to diagnose why a specific attachment failed (e.g. correlating with the external system's rate limits or file size). Response headers are only ever passed through an explicit allowlist (getSafeResponseHeaders) rather than logged wholesale, since attachment fetch responses can carry cookies or other headers that shouldn't end up in logs — this mirrors the same allowlist-over-denylist approach already used for sensitive header stripping elsewhere in the SDK. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.npm run lint.