Add reties delay logic #161
Closed
Conversation
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
WalkthroughAdds a BackoffStrategyDTO value object. Extends Queue::put with an optional BackoffStrategyDTO and passes a serialized backoff payload. Updates qless Lua core to accept/store per-job backoff data, add global backoff defaults, and alter retry handling by rescheduling failed/expired jobs using exponential backoff when configured. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Queue as Queue::put
participant Redis as Redis/Qless (qless.lua)
participant Core as qless.lua Core
App->>Queue: put(class, data, ..., backoffDTO?)
Queue->>Redis: PUT job with payload (backoff JSON if provided)
Redis->>Core: QlessQueue:put(options)
Core-->>Redis: Store job with backoff field (if present)
Redis-->>Queue: jid
Queue-->>App: jid
note over Core,Redis: Later: lock invalidation / retry handling
Core->>Core: Read job.backoff or global defaults
alt initial_delay == 0
Core-->>Core: Immediate retry path (no backoff delay)
else initial_delay > 0
Core->>Core: retry_count from history
Core->>Core: delay = initial_delay * (factor^retry_count)
Core->>Redis: Remove lock, reschedule for now+delay, set state=scheduled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (4 warnings, 1 inconclusive)❌ Failed checks (4 warnings, 1 inconclusive)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/Queues/Queue.php (2)
89-91: Public API: new optional arg — update PHPDoc.Add the new parameter to the method docblock to keep IDE hints accurate.
* @param string[]|null $depends A list of JIDs this job must wait on before executing. + * @param \Qless\Queues\DTO\BackoffStrategyDTO|null $backoffStrategyDTO Optional per-job retry backoff. * @return string The job identifier.
125-127: Don’t always send a backoff payload.Right now an empty JSON array is always sent; the Lua side will treat it as “present”. Either (a) omit the pair when DTO is null (preferred), or (b) decode-and-ignore empties in Lua (see my Lua fix below). If you want to do it here, build the varargs array conditionally.
// Outside this hunk, replace the direct ->put(...) with: $args = [ '', $this->name, $jid, $className, $putData, is_null($delay) ? 0 : $delay, 'priority', is_null($priority) ? 0 : $priority, 'tags', json_encode($tags ?: [], JSON_UNESCAPED_SLASHES), 'retries', is_null($retries) ? 5 : $retries, 'depends', json_encode($depends ?: [], JSON_UNESCAPED_SLASHES), ]; if ($backoffStrategyDTO) { $args[] = 'backoff'; $args[] = json_encode($backoffStrategyDTO->toArray(), JSON_UNESCAPED_SLASHES); } $jid = call_user_func_array([$this->client, 'put'], $args);src/qless-core/qless.lua (1)
1980-1993: Optional: clamp extreme delays.Consider a max delay (configurable) to prevent runaway scheduling for large retry_count.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Queues/DTO/BackoffStrategyDTO.php(1 hunks)src/Queues/Queue.php(3 hunks)src/qless-core/qless.lua(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Queues/Queue.php (1)
src/Queues/DTO/BackoffStrategyDTO.php (2)
BackoffStrategyDTO(5-46)toArray(39-45)
🪛 GitHub Actions: PR checks and tests
src/Queues/Queue.php
[error] 127-127: QlessException: ERR Error running script (call to f_15c3572b35acf0713ccad4d65166bf6c84b5cdfa): @user_script:1608: Lua redis() command arguments must be strings or integers
🔇 Additional comments (3)
src/Queues/DTO/BackoffStrategyDTO.php (1)
39-45: LGTM — array shape matches Lua expectations (snake_case keys).src/Queues/Queue.php (1)
15-15: Import looks correct.src/qless-core/qless.lua (1)
389-392: Config defaults: LGTM.Good defaults with “0 = disabled” semantics for initial delay.
Comment on lines
+1590
to
+1608
| local job_fields = { | ||
| 'jid' , jid, | ||
| 'klass' , klass, | ||
| 'data' , raw_data, | ||
| 'priority' , priority, | ||
| 'tags' , cjson.encode(tags), | ||
| 'state' , ((delay > 0) and 'scheduled') or 'waiting', | ||
| 'worker' , '', | ||
| 'expires' , 0, | ||
| 'queue' , self.name, | ||
| 'retries' , retries, | ||
| 'remaining', retries, | ||
| 'time' , string.format("%.20f", now) | ||
| } | ||
| if options['backoff'] then | ||
| table.insert(job_fields, 'backoff') | ||
| table.insert(job_fields, cjson.encode(options['backoff'])) | ||
| end | ||
| redis.call('hmset', QlessJob.ns .. jid, job_fields) |
There was a problem hiding this comment.
Fix pipeline breaker: HMSET with a table (must unpack).
This is the CI error (“Lua redis() command arguments must be strings or integers”). HMSET needs flat field/value pairs.
- redis.call('hmset', QlessJob.ns .. jid, job_fields)
+ redis.call('hmset', QlessJob.ns .. jid, unpack(job_fields))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local job_fields = { | |
| 'jid' , jid, | |
| 'klass' , klass, | |
| 'data' , raw_data, | |
| 'priority' , priority, | |
| 'tags' , cjson.encode(tags), | |
| 'state' , ((delay > 0) and 'scheduled') or 'waiting', | |
| 'worker' , '', | |
| 'expires' , 0, | |
| 'queue' , self.name, | |
| 'retries' , retries, | |
| 'remaining', retries, | |
| 'time' , string.format("%.20f", now) | |
| } | |
| if options['backoff'] then | |
| table.insert(job_fields, 'backoff') | |
| table.insert(job_fields, cjson.encode(options['backoff'])) | |
| end | |
| redis.call('hmset', QlessJob.ns .. jid, job_fields) | |
| local job_fields = { | |
| 'jid' , jid, | |
| 'klass' , klass, | |
| 'data' , raw_data, | |
| 'priority' , priority, | |
| 'tags' , cjson.encode(tags), | |
| 'state' , ((delay > 0) and 'scheduled') or 'waiting', | |
| 'worker' , '', | |
| 'expires' , 0, | |
| 'queue' , self.name, | |
| 'retries' , retries, | |
| 'remaining', retries, | |
| 'time' , string.format("%.20f", now) | |
| } | |
| if options['backoff'] then | |
| table.insert(job_fields, 'backoff') | |
| table.insert(job_fields, cjson.encode(options['backoff'])) | |
| end | |
| redis.call('hmset', QlessJob.ns .. jid, unpack(job_fields)) |
🤖 Prompt for AI Agents
In src/qless-core/qless.lua around lines 1590-1608, HMSET is being called with
the job_fields table itself which causes the Redis Lua client error; change the
call to pass the field/value pairs as separate arguments by unpacking the
job_fields table (use table.unpack(job_fields)) so HMSET receives flat
string/integer args instead of a table.
Comment on lines
+1604
to
+1607
| if options['backoff'] then | ||
| table.insert(job_fields, 'backoff') | ||
| table.insert(job_fields, cjson.encode(options['backoff'])) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-encoding and ignore empty backoff.
options.backoff arrives JSON-encoded from PHP; encoding again stores a JSON string literal. Decode first and only persist non-empty tables.
- if options['backoff'] then
- table.insert(job_fields, 'backoff')
- table.insert(job_fields, cjson.encode(options['backoff']))
- end
+ if options['backoff'] then
+ local raw = options['backoff']
+ local backoff = (type(raw) == 'string') and (cjson.decode(raw) or {}) or raw
+ if type(backoff) == 'table' and next(backoff) ~= nil then
+ table.insert(job_fields, 'backoff')
+ table.insert(job_fields, cjson.encode(backoff))
+ end
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if options['backoff'] then | |
| table.insert(job_fields, 'backoff') | |
| table.insert(job_fields, cjson.encode(options['backoff'])) | |
| end | |
| if options['backoff'] then | |
| local raw = options['backoff'] | |
| local backoff = (type(raw) == 'string') and (cjson.decode(raw) or {}) or raw | |
| if type(backoff) == 'table' and next(backoff) ~= nil then | |
| table.insert(job_fields, 'backoff') | |
| table.insert(job_fields, cjson.encode(backoff)) | |
| end | |
| end |
🤖 Prompt for AI Agents
In src/qless-core/qless.lua around lines 1604-1607, options['backoff'] is
arriving as a JSON-encoded string so re-encoding it stores a JSON string
literal; decode it first (use pcall/cjson.decode), verify the decoded value is a
non-empty table (next(decoded) ~= nil), and only then add 'backoff' and the
properly encoded JSON of that table to job_fields; if decode fails or the table
is empty, do not insert anything.
Comment on lines
+1966
to
+1971
| local backoff_json = redis.call('hget', Qless.ns .. jid, 'backoff') | ||
| local backoff_config = {} | ||
| if backoff_json then | ||
| backoff_config = cjson.decode(backoff_json) | ||
| end | ||
|
|
There was a problem hiding this comment.
Read backoff from the correct job key.
Using Qless.ns builds “ql:”, not the job hash.
- local backoff_json = redis.call('hget', Qless.ns .. jid, 'backoff')
+ local backoff_json = redis.call('hget', QlessJob.ns .. jid, 'backoff')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local backoff_json = redis.call('hget', Qless.ns .. jid, 'backoff') | |
| local backoff_config = {} | |
| if backoff_json then | |
| backoff_config = cjson.decode(backoff_json) | |
| end | |
| local backoff_json = redis.call('hget', QlessJob.ns .. jid, 'backoff') | |
| local backoff_config = {} | |
| if backoff_json then | |
| backoff_config = cjson.decode(backoff_json) | |
| end |
🤖 Prompt for AI Agents
In src/qless-core/qless.lua around lines 1966–1971, the code reads backoff JSON
from redis.call('hget', Qless.ns .. jid, 'backoff') which concatenates the
namespace and jid (producing "ql:<jid>") instead of the job hash key; change the
key to the job hash (e.g. Qless.ns .. 'jobs:' .. jid or use the existing helper
like Qless.jobKey(jid) if available) so the hget targets the correct job hash,
then decode backoff_json as before.
Comment on lines
+1972
to
+1989
| local initial_delay = tonumber(backoff_config['initial_delay']) | ||
| local backoff_factor = tonumber(backoff_config['factor']) | ||
| if initial_delay == nil then | ||
| initial_delay = tonumber(Qless.config.get('backoff-initial-delay', 0)) | ||
| end | ||
| if backoff_factor == nil then | ||
| backoff_factor = tonumber(Qless.config.get('backoff-factor', 3)) | ||
| end | ||
| if initial_delay == 0 then | ||
| table.insert(jids, jid) | ||
| else | ||
| local job = Qless.job(jid) | ||
| local job_history = job:history() | ||
| local retry_count = #job_history - 1 | ||
| if retry_count < 0 then retry_count = 0 end | ||
|
|
||
| local delay = initial_delay * (backoff_factor ^ retry_count) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Derive retry_count from retries/remaining, not history length.
History length is not equal to attempts and can explode delays. Use (retries - remaining), clamped to >= 0.
- local job = Qless.job(jid)
- local job_history = job:history()
- local retry_count = #job_history - 1
- if retry_count < 0 then retry_count = 0 end
-
- local delay = initial_delay * (backoff_factor ^ retry_count)
+ local total_retries = tonumber(redis.call('hget', QlessJob.ns .. jid, 'retries')) or 0
+ local remaining = tonumber(redis.call('hget', QlessJob.ns .. jid, 'remaining')) or 0
+ local retry_count = math.max(0, total_retries - remaining)
+ local delay = initial_delay * (backoff_factor ^ retry_count)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local initial_delay = tonumber(backoff_config['initial_delay']) | |
| local backoff_factor = tonumber(backoff_config['factor']) | |
| if initial_delay == nil then | |
| initial_delay = tonumber(Qless.config.get('backoff-initial-delay', 0)) | |
| end | |
| if backoff_factor == nil then | |
| backoff_factor = tonumber(Qless.config.get('backoff-factor', 3)) | |
| end | |
| if initial_delay == 0 then | |
| table.insert(jids, jid) | |
| else | |
| local job = Qless.job(jid) | |
| local job_history = job:history() | |
| local retry_count = #job_history - 1 | |
| if retry_count < 0 then retry_count = 0 end | |
| local delay = initial_delay * (backoff_factor ^ retry_count) | |
| local initial_delay = tonumber(backoff_config['initial_delay']) | |
| local backoff_factor = tonumber(backoff_config['factor']) | |
| if initial_delay == nil then | |
| initial_delay = tonumber(Qless.config.get('backoff-initial-delay', 0)) | |
| end | |
| if backoff_factor == nil then | |
| backoff_factor = tonumber(Qless.config.get('backoff-factor', 3)) | |
| end | |
| if initial_delay == 0 then | |
| table.insert(jids, jid) | |
| else | |
| local total_retries = tonumber(redis.call('hget', QlessJob.ns .. jid, 'retries')) or 0 | |
| local remaining = tonumber(redis.call('hget', QlessJob.ns .. jid, 'remaining')) or 0 | |
| local retry_count = math.max(0, total_retries - remaining) | |
| local delay = initial_delay * (backoff_factor ^ retry_count) |
🤖 Prompt for AI Agents
In src/qless-core/qless.lua around lines 1972 to 1989, the code computes
retry_count from job history length which can be incorrect and inflate backoff;
instead fetch the job's configured retries and current remaining attempts and
compute retry_count = math.max(0, retries - remaining). Update the code to get
numeric retries and remaining from the job (e.g. call the appropriate job API to
read retries and remaining, coerce to numbers), clamp to >= 0, and use that
retry_count when calculating delay.
Comment on lines
+17
to
+21
| public function __construct(int $initialDelay, int $factor) | ||
| { | ||
| $this->initialDelay = $initialDelay; | ||
| $this->factor = $factor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate inputs (non-negative delay, factor >= 1).
Protect against misconfiguration early.
public function __construct(int $initialDelay, int $factor)
{
+ if ($initialDelay < 0) {
+ throw new \InvalidArgumentException('initialDelay must be >= 0');
+ }
+ if ($factor < 1) {
+ throw new \InvalidArgumentException('factor must be >= 1');
+ }
$this->initialDelay = $initialDelay;
$this->factor = $factor;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function __construct(int $initialDelay, int $factor) | |
| { | |
| $this->initialDelay = $initialDelay; | |
| $this->factor = $factor; | |
| } | |
| public function __construct(int $initialDelay, int $factor) | |
| { | |
| if ($initialDelay < 0) { | |
| throw new \InvalidArgumentException('initialDelay must be >= 0'); | |
| } | |
| if ($factor < 1) { | |
| throw new \InvalidArgumentException('factor must be >= 1'); | |
| } | |
| $this->initialDelay = $initialDelay; | |
| $this->factor = $factor; | |
| } |
🤖 Prompt for AI Agents
In src/Queues/DTO/BackoffStrategyDTO.php around lines 17 to 21, the constructor
does not validate inputs; add checks to ensure $initialDelay is non-negative and
$factor is >= 1 before assigning to properties, and throw an
InvalidArgumentException (with clear messages like "initialDelay must be >= 0"
and "factor must be >= 1") when validations fail so misconfiguration is caught
early; perform validation first, then set $this->initialDelay and $this->factor
only if values pass.
YaroslavStryhun
changed the title
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.
Hello!
In raising this pull request, I confirm the following (please check boxes):
Small description of change:
Thanks
Summary by CodeRabbit