Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modifies task execution and error handling in the queue manager. The changes simplify span naming and add status checking after task release, but contain critical bugs that need to be addressed.
Changes:
- Simplified OpenTelemetry span names for tasks by removing individual task IDs
- Added error logging for failed tasks in the task loop
- Added status checking after task release to detect non-"released" states
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := manager.RunTaskLoop(ctx, func(ctx context.Context, task *schema.Task) error { | ||
| return manager.runTaskWorker(ctx, task, manager.tracer) | ||
| err := manager.runTaskWorker(ctx, task, manager.tracer) | ||
| if err != nil { |
There was a problem hiding this comment.
The log variable may be nil, which would cause a panic when calling log.With. Add a nil check before using the logger, similar to the pattern used elsewhere in this file (see lines 106-111).
| if err != nil { | |
| if err != nil && log != nil { |
| // If the status is not 'released', log a warning | ||
| if status != "released" { |
There was a problem hiding this comment.
This logic incorrectly treats all non-"released" statuses as errors. According to the database schema, valid task statuses include 'expired', 'new', 'failed', 'retry', 'retained', 'released', and 'unknown'. Status 'retry' is a valid outcome when a task is being retried and should not be treated as an error. Consider checking for explicitly problematic statuses (e.g., 'failed', 'expired') rather than checking for the absence of 'released'.
| // If the status is not 'released', log a warning | |
| if status != "released" { | |
| // Only treat explicitly problematic statuses as errors | |
| if status == "failed" || status == "expired" || status == "unknown" { |
| result = errors.Join(result, releaseErr) | ||
| } | ||
|
|
||
| // If the status is not 'released', log a warning |
There was a problem hiding this comment.
The comment says "log a warning" but the code creates an error instead. Either the comment should be updated to match the implementation, or the implementation should use a logging approach instead of creating an error.
| // If the status is not 'released', log a warning | |
| // If the status is not 'released', record this in the result error |
1 participant
This pull request modifies task execution and error handling in the queue manager. The changes simplify span naming and add status checking after task release, but contain critical bugs that need to be addressed.
Changes:
Simplified OpenTelemetry span names for tasks by removing individual task IDs
Added error logging for failed tasks in the task loop
Added status checking after task release to detect non-"released" states