Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR hardens error handling for Bungie API interactions by introducing better error classification and retry logic, along with improved logging and diagnostics.
- Adds a new
UnretryableErrortype to distinguish permanent failures from transient errors in message processing - Implements
IsTransientErrorfunction to classify Bungie API errors as retryable or permanent - Enhances error logging by extracting HTML titles from non-JSON responses for better debugging
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/web/bungie/client.go | Adds HTML title extraction for non-JSON responses and implements IsTransientError to classify API errors |
| lib/services/character/fill.go | Refactors error handling to use new transient error classification and extracts database update logic into helper function |
| lib/messaging/processing/topic.go | Introduces UnretryableError type and helper functions for distinguishing permanent vs transient failures |
| apps/hermes/worker.go | Updates message processing to requeue transient errors but send permanent failures to DLQ |
| lib/services/pgcr_processing/request.go | Adds HTTP 502 Bad Gateway handling |
| tools/process-missed-pgcrs/main.go | Improves logging for line parsing errors and applies formatting fixes |
| tools/manifest-downloader/main.go | Removes extraneous blank lines |
| lib/utils/logging/flags.go | Removes unnecessary parentheses and applies consistent formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
lib/messaging/queue-workers/topic.go.template:139
- Unreachable code: Lines 120-138 are unreachable because line 118 returns unconditionally. The commented-out alternative implementations (Options 2 and 3) will never be reached.
Consider restructuring the template to make it clear that only one option should be used:
// processYourTopicName handles messages for your topic
func processYourTopicName(worker processing.WorkerInterface, message amqp.Delivery) error {
// Option 1: JSON message (recommended) - UNCOMMENT TO USE
// request, err := processing.ParseJSON[messages.YourMessageType](worker, message.Body)
// if err != nil {
// return err // ParseJSON already logs the error
// }
//
// worker.Info("PROCESSING_MESSAGE", map[string]any{
// "id": request.ID, // Adjust field names to match your message type
// })
//
// // TODO: Implement your business logic here
// return nil
// Option 2: Int64 message (for simple ID-based messages) - UNCOMMENT TO USE
// id, err := processing.ParseInt64(worker, message.Body)
// ...
// Option 3: Text message - UNCOMMENT TO USE
// text, err := processing.ParseText(worker, message.Body)
// ...
// TEMPLATE: Remove this panic and uncomment one of the options above
panic("template not implemented - choose and uncomment one of the options above")
}func processYourTopicName(worker processing.WorkerInterface, message amqp.Delivery) error {
// Option 1: JSON message (recommended)
request, err := processing.ParseJSON[messages.YourMessageType](worker, message.Body)
if err != nil {
return err // ParseJSON already logs the error
}
worker.Info("PROCESSING_MESSAGE", map[string]any{
"id": request.ID, // Adjust field names to match your message type
})
// Access resources via worker:
// - worker.Context() for cancellation/timeouts
// - Use domain services for business logic (e.g., player.Crawl, character.Fill)
// TODO: Implement your business logic here
// - Call domain service functions
// - Handle errors appropriately:
// - Return error for transient failures (will be retried)
// - Return processing.NewUnretryableError(err) for permanent failures (goes to DLQ)
// Example error handling:
// if err != nil {
// worker.Error("OPERATION_FAILED", err, map[string]any{
// logging.MEMBERSHIP_ID: request.MembershipId,
// })
// return err // Transient error - will retry
// // OR: return processing.NewUnretryableError(err) // Permanent error - goes to DLQ
// }
return nil
// Option 2: Int64 message (for simple ID-based messages) - uncomment to use
// id, err := processing.ParseInt64(worker, message.Body)
// if err != nil {
// return err
// }
//
// // TODO: Implement your business logic here
//
// return nil
// Option 3: Text message - uncomment to use
// text, err := processing.ParseText(worker, message.Body)
// if err != nil {
// return err
// }
//
// // TODO: Implement your business logic here
//
// return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // x-death is an array of death records, one for each time the message was nacked | ||
| xDeath, ok := message.Headers["x-death"].([]any) | ||
| if !ok { | ||
| return 0 | ||
| } | ||
|
|
||
| // Count the number of death records (each represents a nack with requeue=true) | ||
| return len(xDeath) |
There was a problem hiding this comment.
Incorrect retry count implementation: The x-death header in RabbitMQ contains an array with one entry per unique queue the message died in, not one entry per retry. Each death record has a count field that indicates how many times the message died in that specific queue.
The correct implementation should extract the count from the first death record:
func GetRetryCount(message amqp.Delivery) int {
if message.Headers == nil {
return 0
}
xDeath, ok := message.Headers["x-death"].([]interface{})
if !ok || len(xDeath) == 0 {
return 0
}
// Get the first death record (for the current queue)
death, ok := xDeath[0].(amqp.Table)
if !ok {
return 0
}
// Extract the count field which tracks redeliveries
count, ok := death["count"].(int64)
if !ok {
return 0
}
return int(count)
}| // x-death is an array of death records, one for each time the message was nacked | |
| xDeath, ok := message.Headers["x-death"].([]any) | |
| if !ok { | |
| return 0 | |
| } | |
| // Count the number of death records (each represents a nack with requeue=true) | |
| return len(xDeath) | |
| xDeath, ok := message.Headers["x-death"].([]any) | |
| if !ok || len(xDeath) == 0 { | |
| return 0 | |
| } | |
| // Get the first death record (for the current queue) | |
| death, ok := xDeath[0].(amqp.Table) | |
| if !ok { | |
| return 0 | |
| } | |
| // Extract the count field which tracks redeliveries | |
| count, ok := death["count"].(int64) | |
| if !ok { | |
| return 0 | |
| } | |
| return int(count) |
| } | ||
|
|
||
| // NewUnretryableError wraps an error to indicate it should NOT be retried | ||
| func NewUnretryableError(err error) *UnretryableError { |
There was a problem hiding this comment.
Missing nil check: NewUnretryableError doesn't check if err is nil before wrapping it. This could lead to confusing error states where an UnretryableError wraps nil.
Consider adding a nil check:
func NewUnretryableError(err error) *UnretryableError {
if err == nil {
return nil
}
return &UnretryableError{Err: err}
}Alternatively, document that passing nil is not allowed and will result in undefined behavior.
| func NewUnretryableError(err error) *UnretryableError { | |
| func NewUnretryableError(err error) *UnretryableError { | |
| if err == nil { | |
| return nil | |
| } |
| } | ||
| return firstSeen, false, nil | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Logic issue: When historyResult.Success is true and historyResult.Data != nil, but activities is empty (length 0), the function falls through to the final return defaultTime, false, nil instead of returning within the success block. This creates ambiguous logic flow.
Consider restructuring to explicitly return in all success paths:
} else if historyResult.Success {
// Determine first_seen from oldest activity
if historyResult.Data != nil && len(historyResult.Data.Activities) > 0 {
activities := historyResult.Data.Activities
// Activities are ordered newest first, so the last one is the oldest
oldestActivity := activities[len(activities)-1]
firstSeen, parseErr := time.Parse(time.RFC3339, oldestActivity.Period)
if parseErr != nil {
return defaultTime, false, parseErr
}
return firstSeen, false, nil
}
// No activities found, but call was successful
return defaultTime, false, nil
}| } else { | |
| // No activities found, but call was successful |
|
|
||
| // All other errors are transient by default - log as warning | ||
| logger.Warn("ACTIVITY_HISTORY_FETCH_FAILED", err, logFields) | ||
| return defaultTime, false, err |
There was a problem hiding this comment.
Same issue as in character/fill.go: Potential nil error being passed to logging and returned. When err is nil but the request is unsuccessful, this code passes nil to logger.Error/Warn and processing.NewUnretryableError.
The condition } else if err != nil { at line 180 suggests this branch only executes when err is not nil, but if that's the case, the earlier branches (lines 161-179) should have explicit handling when err is nil but the request failed.
| return defaultTime, false, err | |
| return defaultTime, false, err | |
| } else { | |
| // Unsuccessful request, but err is nil: treat as unretryable error and log | |
| logFields := map[string]any{ | |
| logging.MEMBERSHIP_ID: membershipId, | |
| "bungie_error_code": historyResult.BungieErrorCode, | |
| "http_status_code": historyResult.HttpStatusCode, | |
| } | |
| errMsg := fmt.Errorf("activity history fetch failed: BungieErrorCode=%d, HttpStatusCode=%d", historyResult.BungieErrorCode, historyResult.HttpStatusCode) | |
| logger.Error("ACTIVITY_HISTORY_FETCH_ERROR", errMsg, logFields) | |
| return defaultTime, false, processing.NewUnretryableError(errMsg) |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
This pull request introduces a robust and configurable retry and error handling system for queue workers, along with improvements to logging and documentation. The main focus is to ensure that transient errors are retried up to a configurable maximum, while permanent (unretryable) errors are sent directly to the dead letter queue (DLQ). The changes also enhance logging with retry counts and clarify error handling best practices.
Error Handling and Retry System:
UnretryableErrortype and helper functions (NewUnretryableError,IsUnretryableError) to clearly distinguish between transient (retryable) and permanent (unretryable) errors. This ensures that only permanent failures bypass retries and are sent to the DLQ.GetRetryCountto extract the retry count from RabbitMQ message headers, enabling precise control and logging of message retry attempts.worker.goto:MaxRetryCountlimit, sending messages to the DLQ if exceeded, regardless of error type.requeue=truefor retryable errors andrequeue=falsefor unretryable errors. [1] [2] [3] [4]Configuration and Topic Updates:
MaxRetryCounttoTopicConfigand set appropriate retry limits for all major queue topics (e.g.,instance_store,player_crawl,pgcr_crawl, etc.), allowing fine-grained control over retry behavior per queue. [1] [2] [3] [4] [5] [6] [7] [8] [9]Documentation and Logging Improvements:
ARCHITECTURE.mdwith a new section detailing error handling, retry mechanisms, configuration options, and best practices for transient vs. permanent errors.LOGGING.mdto describe the log format, timestamp precision, and consistent handling of error fields (including nil errors), making logs easier to query and analyze. [1] [2]Worker Scaling Improvements:
player_crawltopic to use more aggressive scaling parameters for faster response to queue depth changes.Miscellaneous:
These changes collectively make the queue worker system more resilient, configurable, and easier to monitor and debug.