Bug
When workUnit.UnmarshalJob() returns an error inside makeRetryDecision, the error is only logged — execution continues to call workUnit.Timeout() and workUnit.NextRetry() on an incompletely initialized work unit.
Affected file
internal/maintenance/job_rescuer.go, lines 322–326:
workUnit := workUnitFactory.MakeUnit(job)
if err := workUnit.UnmarshalJob(); err != nil {
s.Logger.ErrorContext(ctx, s.Name+": Error unmarshaling job args: %s"+err.Error(),
slog.String("job_kind", job.Kind), slog.Int64("job_id", job.ID))
}
// execution falls through to here even on unmarshal failure:
timeout := workUnit.Timeout()Impact
workUnit.Timeout() returns its zero value (0), which passes the timeout < 0 || timeout > 0 && ... guard as neither branch fires — the jobRetryDecisionIgnore path is skipped, so the job is never left running when it should be.workUnit.NextRetry() also returns zero time, falling back to ClientRetryPolicy, silently losing any worker-specific retry override.
The job gets an incorrect rescue decision (reschedule with wrong timing) instead of the discard-with-error path that nil workUnitFactory already uses correctly.
There's also a minor string formatting issue on line 323: ": Error unmarshaling job args: %s" + err.Error() embeds a literal %s in the log string instead of using a slog attribute.
Suggested fix
Return early on unmarshal failure, matching the pattern used for the nil-factory case above:
if err := workUnit.UnmarshalJob(); err != nil {
s.Logger.ErrorContext(ctx, s.Name+": Error unmarshaling job args",
slog.String("job_kind", job.Kind), slog.Int64("job_id", job.ID),
slog.String("error", err.Error()))
return jobRetryDecisionDiscard, time.Time{}
}(Whether to discard or retry with a delay is a policy call — discard mirrors the existing nil-factory behavior, but a retry with backoff could also be appropriate.)
Bug
When
workUnit.UnmarshalJob()returns an error insidemakeRetryDecision, the error is only logged — execution continues to callworkUnit.Timeout()andworkUnit.NextRetry()on an incompletely initialized work unit.Affected file
internal/maintenance/job_rescuer.go, lines 322–326:Impact
workUnit.Timeout()returns its zero value (0), which passes thetimeout < 0 || timeout > 0 && ...guard as neither branch fires — thejobRetryDecisionIgnorepath is skipped, so the job is never left running when it should be.workUnit.NextRetry()also returns zero time, falling back toClientRetryPolicy, silently losing any worker-specific retry override.The job gets an incorrect rescue decision (reschedule with wrong timing) instead of the discard-with-error path that
nil workUnitFactoryalready uses correctly.There's also a minor string formatting issue on line 323:
": Error unmarshaling job args: %s" + err.Error()embeds a literal%sin the log string instead of using a slog attribute.Suggested fix
Return early on unmarshal failure, matching the pattern used for the nil-factory case above:
(Whether to discard or retry with a delay is a policy call — discard mirrors the existing nil-factory behavior, but a retry with backoff could also be appropriate.)