Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/messaging/queue-workers/activity_history.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func ActivityHistoryTopic() processing.Topic {
ScaleUpPercent: 0.2,
ScaleDownPercent: 0.1,
BungieSystemDeps: []string{"Destiny2", "Activities", "D2Profiles"},
MaxRetryCount: 3,
MaxRetryCount: 0,
RetryDelay: processing.ExponentialRetryDelay(time.Second),
}, processActivityHistory)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/messaging/queue-workers/character_fill.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func CharacterFillTopic() processing.Topic {
ScaleUpPercent: 0.2,
ScaleDownPercent: 0.1,
BungieSystemDeps: []string{"Destiny2", "D2Characters"},
MaxRetryCount: 4, // Character data is useful but not critical
MaxRetryCount: 0,
RetryDelay: processing.ExponentialRetryDelay(5 * time.Minute),
}, processCharacterFill)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/messaging/queue-workers/clan_crawl.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func ClanCrawlTopic() processing.Topic {
ScaleUpPercent: 0.2,
ScaleDownPercent: 0.1,
BungieSystemDeps: []string{"Groups", "Clans", "Destiny2"},
MaxRetryCount: 5,
MaxRetryCount: 0,
RetryDelay: processing.ExponentialRetryDelay(time.Second),
}, processClanCrawl)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/messaging/queue-workers/player_crawl.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func PlayerCrawlTopic() processing.Topic {
ConsecutiveChecksDown: 3, // More conservative for scale-down
ScaleCooldown: 30 * time.Second, // Shorter cooldown for faster scaling
BungieSystemDeps: []string{"Destiny2", "D2Profiles", "Activities"},
MaxRetryCount: 5, // Reduced from 12 to prevent exponential retry amplification
MaxRetryCount: 0, // Unlimited queue backoff (30m cap); CF blocks retry via delayed exchange
RetryDelay: processing.ExponentialRetryDelay(5 * time.Minute),
}, processPlayerCrawl)
}
Expand Down
15 changes: 7 additions & 8 deletions lib/utils/network/retry.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,15 @@ func TransientNetworkErrorRetryConfig() retry.RetryConfig {
}
}

// Uses more attempts and longer delays to handle Cloudflare's rate limiting and blocking pages
// Only retries if the error is a Cloudflare error
// Params: logger: logger to use for logging, loggingFields: fields to add to the logging
// CloudflareRetryConfig performs one quick retry after ~2s for Cloudflare challenge pages.
// Longer outages are handled by Hermes queue backoff.
func CloudflareRetryConfig(logger logging.Logger, loggingFields map[string]any) retry.RetryConfig {
return retry.RetryConfig{
MaxAttempts: 3,
InitialDelay: 1 * time.Second,
MaxDelay: 10 * time.Second,
Multiplier: 4, // Back off fast
Jitter: 0.2, // 20% jitter for better distribution of retries
MaxAttempts: 1,
InitialDelay: 2 * time.Second,
MaxDelay: 2 * time.Second,
Multiplier: 1.0,
Jitter: 0.2,
OnRetry: func(attempt int, err error) {
fields := map[string]any{
logging.ATTEMPTS: attempt,
Expand Down
20 changes: 8 additions & 12 deletions lib/web/bungie/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ func (e *BungieResponseParseError) Error() string {
return fmt.Sprintf("%s: unexpected bungie %d %s response: '%s'", e.Operation, e.StatusCode, e.ContentType, e.Title)
}

// isKnownHTMLErrorPage checks if the error is a known HTML error page that should not be sent to Sentry
// These are expected/recoverable conditions that are handled by retry logic
// isKnownHTMLErrorPage checks if the error is a known HTML error page that should not be sent to Sentry.
// Cloudflare pages get one quick in-process retry; longer blocks use Hermes queue backoff.
func (e *BungieResponseParseError) isKnownHTMLErrorPage() bool {
if e.ContentType == "" || e.Title == "" {
return false
Expand Down Expand Up @@ -97,12 +97,9 @@ func (e *BungieResponseParseError) isKnownHTMLErrorPage() bool {
}

func get[T any](ctx context.Context, c *BungieClient, url netUrl.URL, operation string, params map[string]any) (BungieHttpResult[T], error) {
// Wraps the get in 2 layers of retry:
// Inner layer retries timeout and connection errors (excludes BungieError instances)
// Outer layer retries Cloudflare errors
// Outer: one ~2s Cloudflare retry. Inner: up to two quick transient retries. Queue handles the rest.
return retry.WithRetryForResult(ctx, network.CloudflareRetryConfig(clientLogger, params), func(attempt int) (BungieHttpResult[T], error) {
if attempt > 1 {
// add a query parameter to the url to indicate the retry attempt
queryValues := url.Query()
queryValues.Add("retry", fmt.Sprintf("%d", attempt))
url.RawQuery = queryValues.Encode()
Comment on lines 103 to 105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The condition if attempt > 1 is unreachable because the retry logic is configured with MaxAttempts: 1, making the code to add a retry query parameter dead.
Severity: LOW

Suggested Fix

Either remove the unreachable if attempt > 1 block to eliminate the dead code, or if the retry query parameter is desired for observability, increase the MaxAttempts value in the CloudflareRetryConfig to be greater than 1.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: lib/web/bungie/client.go#L103-L105

Potential issue: In the retry logic for Bungie API requests, a condition `if attempt >
1` is used to add a `retry` query parameter to the URL. However, the underlying retry
mechanism is configured with `MaxAttempts: 1`. This configuration means the function
will be called with `attempt = 0` on the first try and `attempt = 1` on the single retry
attempt. The `attempt` variable will never be greater than 1, making the conditional
block unreachable dead code. This results in a minor loss of observability, as the
intended debugging parameter will never be added.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -289,17 +286,16 @@ func IsTransientError(bungieErrorCode int, httpStatusCode int) bool {
return true
}

// BungieRetryConfig retries transient network errors for Bungie API calls
// It specifically excludes BungieError instances (application-level errors) from retries
// such as timeout, connection errors, and server errors (5xx)
// BungieRetryConfig retries transient network errors for Bungie API calls (timeout, connection, 5xx).
// At most two quick retries; Cloudflare and longer outages are handled by Hermes queue backoff.
func BungieRetryConfig() retry.RetryConfig {
transientRetryConfig := network.TransientNetworkErrorRetryConfig()
return retry.RetryConfig{
MaxAttempts: 3,
MaxAttempts: 2,
InitialDelay: 50 * time.Millisecond,
MaxDelay: 5 * time.Second,
MaxDelay: 2 * time.Second,
Multiplier: 2.0,
Jitter: 0.1, // 10% jitter
Jitter: 0.1,
OnRetry: nil,
ShouldRetry: func(err error) bool {
// Check if this is a Bungie error (application-level error, not a network error)
Expand Down
Loading