feat(core): integrate RetryManager into SegmentDestination upload pipeline#1160
Open
abueide wants to merge 13 commits intotapi/retry-manager-testsfrom
Open
feat(core): integrate RetryManager into SegmentDestination upload pipeline#1160abueide wants to merge 13 commits intotapi/retry-manager-testsfrom
abueide wants to merge 13 commits intotapi/retry-manager-testsfrom
Conversation
ba89956 to
aac6169
Compare
af03054 to
872790a
Compare
aac6169 to
cf2abd2
Compare
872790a to
f6282d0
Compare
cf2abd2 to
23ae940
Compare
f6282d0 to
a49f9ec
Compare
23ae940 to
9f2575b
Compare
2b7adb3 to
3fb2dce
Compare
9f2575b to
6981059
Compare
3fb2dce to
ef94825
Compare
6981059 to
c49e640
Compare
ef94825 to
1be5046
Compare
4 tasks
1be5046 to
bd77e18
Compare
02cca5f to
42d7cd1
Compare
bd77e18 to
2f3fd0f
Compare
42d7cd1 to
6bf54b1
Compare
fa537b0 to
fb0788b
Compare
f52a425 to
e83d5ab
Compare
fb0788b to
78b3bec
Compare
e83d5ab to
91c9ce3
Compare
78b3bec to
556e28a
Compare
91c9ce3 to
7242cbf
Compare
6efe106 to
731d4ae
Compare
7242cbf to
b60f971
Compare
eecc8d3 to
34aeb65
Compare
…eline Wire RetryManager into SegmentDestination for TAPI-compliant retry handling: uploadBatch() with error classification, event pruning on partial failures, retry count header propagation, and QueueFlushingPlugin error type handling updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Don't reset retry state on partial success when concurrent batches have 429/transient errors - Use if/else if so 429 takes precedence over transient error handling - Remove redundant return Promise.resolve() in async function - Fix duplicate keepalive property from master merge Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use res.ok instead of res.status === 200 for 2xx success range - Remove dead dequeue() method from QueueFlushingPlugin - Add destroy() to SegmentDestination for RetryManager timer cleanup - Reset retry state when queue is empty at flush time or after pruning - Call handle429 per result instead of pre-aggregating, so RetryManager.applyRetryStrategy respects eager/lazy consolidation - Simplify retryAfterSeconds fallback to ?? 60 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ination Extract pruneExpiredEvents() and updateRetryState() from sendEvents to improve readability. Remove redundant/obvious comments, merge duplicate switch cases, and simplify return statements throughout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… pruning - Override shutdown() instead of standalone destroy() to integrate with the plugin lifecycle — prevents auto-flush timer leak on client cleanup - Handle RetryResult 'limit_exceeded' from RetryManager: log warning and let per-event age pruning (pruneExpiredEvents via _queuedAt) handle event drops rather than dropping all retryable events on global counter reset - Import RetryResult type for type-safe limit checking Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Root cause: when the CDN settings response had no httpConfig field, analytics.ts guarded the entire merge block with if (resJson.httpConfig), so this.httpConfig stayed undefined. This prevented RetryManager creation in SegmentDestination, disabling all retry features (error classification overrides, canRetry() gating, retry counting, maxRetries enforcement). Changes: - Remove httpConfig guard in analytics.ts — always merge defaultHttpConfig as baseline, with CDN and config.httpConfig overrides on top - Add httpConfig?: DeepPartial<HttpConfig> to Config type for client-side overrides (e.g. maxRetries from test harness) - Drop retryable events when retry limits exceeded in SegmentDestination instead of leaving them in the queue indefinitely - Update fetchSettings test to expect defaultHttpConfig when CDN has no httpConfig Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion tests Remove autoFlushOnRetryReady check, setAutoFlushCallback call, and shutdown() method. Add tests for CDN integrations validation (null, array, string, and no-defaults scenarios). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire up the droppedEventCount counter (added in cli-flush-retry-loop) at the two places SegmentDestination permanently removes events: permanent errors and retry limit exceeded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When CDN returns a valid 200 with no integrations field (e.g. {}),
treat it as authoritative "no integrations configured" rather than
falling back to defaultSettings. This ensures SegmentDestination is
correctly disabled when the server has no integrations.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… as drops - Remove retryStrategy from Config type, defaultConfig, and RetryManager constructor — always use eager (shortest wait) for concurrent batch errors - Count events pruned by maxTotalBackoffDuration toward droppedEventCount - Add SDD-default config tests verifying all spec status codes against defaultHttpConfig overrides (408/410/460=retry, 501/505=drop) - Update RetryManager tests for eager-only behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Property declaration was lost during branch rebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b60f971 to
e00cdf7
Compare
34aeb65 to
0db00e3
Compare
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.
Summary
RetryManagerintoSegmentDestinationfor TAPI-compliant retry handlinguploadBatch()with error classification, retry-after parsing, and configurable retry behavioraggregateErrors()to separate batch results by status (success/429/transient/permanent)updateRetryState()that delegates to RetryManager and returns whether limits were exceeded_queuedAttimestamps andpruneExpiredEvents()— events older thanmaxTotalBackoffDurationare dropped individuallycanRetry()upload gate check before sending batchesX-Retry-Countheader propagation to API requestsQueueFlushingPlugin.dequeue()withdequeueByMessageIds()for messageId-based dequeueisPendingUploadflag with promise-based concurrent flush guardshutdown()(instead of standalonedestroy()) to integrate with plugin lifecycle — prevents auto-flush timer leak on client cleanupRetryResult.limit_exceeded: log warning and let per-event age pruning handle drops (events are NOT bulk-dropped on global counter reset)PR 5 of 5 in the TAPI backoff/retry stack. Depends on #1159.
Test plan
shutdown()properly cleans up RetryManager timers via plugin lifecycle_queuedAtage, not by global retry counter reset🤖 Generated with Claude Code